remove _warn_and_correct_transformer_size funct #10819

emysdias · 2022-02-06T21:21:37Z

Proposed changes:

Removes unusual function as you can see the issue here: remove _warn_and_correct_transformer_size as it's not needed anymore #10712

Status (please check what you already did):

added some tests for the functionality
updated the documentation
updated the changelog (please check changelog for instructions)
reformat files using black (please check Readme for instructions)

CLAassistant · 2022-02-06T21:21:41Z

All committers have signed the CLA.

Co-authored-by: Washington <[email protected]>

emysdias · 2022-02-09T03:35:44Z

@jupyterjazz can you check later please? And I don't know why codeclimate failed if I didn't change these files :(

jupyterjazz

Good job 🚀 I'm not sure why codeclimate is failing. I'll look at it later, but before that we still need to change few things:

Add a changelog file. You can see how it's done here.
Update PR description and link the original issue there
After all these changes are done, we can work on a better testing.

jupyterjazz · 2022-02-09T11:21:41Z

rasa/nlu/selectors/response_selector.py

@@ -414,7 +393,7 @@ def _check_config_params_when_transformer_enabled(self) -> None:
                f"({self.retrieval_intent})" if self.retrieval_intent else ""
            )
            self._warn_about_transformer_and_hidden_layers_enabled(selector_name)
-            self._warn_and_correct_transformer_size(selector_name)
+            self.component_config[TRANSFORMER_SIZE] = DEFAULT_TRANSFORMER_SIZE


We don't need this line here as the actual correction happens in _get_transformer_dimensions

thanks for your suggestion, but when I delete this line, I get 1 failed test

t

and 4 errors in def test_sets_integer_transformer_size_when_needed, like this:

I'm aware of the error. Tests fail because we don't modify self.component_config anymore so it keeps the old value. However this line still should be removed because correction happens in _get_transformer_dimensions. We just need to come up with a different way of testing it. You can experiment a bit if you want to figure this out yourself, but if you find yourself stuck just ping me and I'll give you some insights.

ok, thanks for the suggestions!!

@jupyterjazz I'm stuck hahaha I'm trying to do this issue with a friend, it was our first issue in Rasa, so we don't know how to fix this tests :(

jupyterjazz · 2022-02-09T11:33:05Z

rasa/utils/tensorflow/rasa_layers.py

+                f"`{TRANSFORMER_SIZE}` is set to "
+                f"`{transformer_layers}` for "
+                f"{attribute}, but a positive size is required when using "
+                f"`{NUM_TRANSFORMER_LAYERS} > 0`. {attribute} will proceed, using "
+                f"`{TRANSFORMER_SIZE}={DEFAULT_TRANSFORMER_SIZE}`. "
+                f"Alternatively, specify a different value in the component's config.",
+                category=UserWarning,
+            )


_get_transformer_dimensions can be called by three components ResponseSelector, DIETClassifier and TEDPolicy. Since we don't know which one is calling (although correction would only happen inside ResponseSelector because others have positive TRANSFORMER_SIZE by default) we should change the warning message.

Suggested change

f"`{TRANSFORMER_SIZE}` is set to "

f"`{transformer_layers}` for "

f"{attribute}, but a positive size is required when using "

f"`{NUM_TRANSFORMER_LAYERS} > 0`. {attribute} will proceed, using "

f"`{TRANSFORMER_SIZE}={DEFAULT_TRANSFORMER_SIZE}`. "

f"Alternatively, specify a different value in the component's config.",

category=UserWarning,

)

f"`{TRANSFORMER_SIZE}` is set to "

f"`{transformer_units}`, "

f"but a positive size is required when using "

f"`{NUM_TRANSFORMER_LAYERS} > 0`. We will proceed, using "

f"`{TRANSFORMER_SIZE}={DEFAULT_TRANSFORMER_SIZE}`. "

f"Alternatively, specify a different value in the component's config.",

category=UserWarning,

)

This way it's a bit non-specific but still understandable.

…and_correct_transformer_size

jupyterjazz · 2022-02-10T14:13:45Z

CHANGELOG.mdx

+### Miscellaneous internal changes
+- [#10712](https://github.com/RasaHQ/rasa/issues/10712): remove _warn_and_correct_transformer_size function as it's not needed anymore
+


You shouldn't directly edit CHANGELOG.mdx. Here's one example of a changelog file:

Please follow the instructions given here and create a similar file.

jupyterjazz · 2022-02-10T14:28:44Z

rasa/nlu/selectors/response_selector.py

@@ -414,7 +393,7 @@ def _check_config_params_when_transformer_enabled(self) -> None:
                f"({self.retrieval_intent})" if self.retrieval_intent else ""
            )
            self._warn_about_transformer_and_hidden_layers_enabled(selector_name)
-            self._warn_and_correct_transformer_size(selector_name)
+            self.component_config[TRANSFORMER_SIZE] = DEFAULT_TRANSFORMER_SIZE


I'm aware of the error. Tests fail because we don't modify self.component_config anymore so it keeps the old value. However this line still should be removed because correction happens in _get_transformer_dimensions. We just need to come up with a different way of testing it. You can experiment a bit if you want to figure this out yourself, but if you find yourself stuck just ping me and I'll give you some insights.

emysdias · 2022-02-19T19:45:57Z

Just to let you know that I'm going to go back with this PR and @WashingtonBispo is going to help too

…and_correct_transformer_size

losterloh · 2022-06-01T09:05:30Z

hey @emysdias , is this still relevant / are you still planning on working on this? 🙂

emysdias · 2022-06-01T23:38:02Z

hey @emysdias , is this still relevant / are you still planning on working on this? slightly_smiling_face

I dont know, I got no answer anymore and not going to work on this anymore

m-vdb · 2023-05-25T07:10:39Z

Hi there! Thanks for your contribution. As the PR has been stale for a while, I'm closing it. Please feel free to reopen an issue / PR if the problem still occurs on latest rasa versions.

emysdias requested review from a team and kedz and removed request for a team February 6, 2022 21:21

emysdias force-pushed the remove_warn_and_correct_transformer_size branch from d2b52a9 to 402ee76 Compare February 6, 2022 21:26

emysdias marked this pull request as draft February 6, 2022 22:07

emysdias marked this pull request as ready for review February 8, 2022 01:06

emysdias marked this pull request as draft February 8, 2022 02:37

emysdias force-pushed the remove_warn_and_correct_transformer_size branch from 7e77db2 to db5d282 Compare February 9, 2022 01:05

fix code climate error

9db1876

Co-authored-by: Washington <[email protected]>

emysdias force-pushed the remove_warn_and_correct_transformer_size branch from db5d282 to 9db1876 Compare February 9, 2022 01:25

emysdias and others added 2 commits February 8, 2022 23:18

remove if cond

88c1b11

fix test

5763323

Co-authored-by: Washington <[email protected]>

emysdias marked this pull request as ready for review February 9, 2022 03:31

jupyterjazz requested review from jupyterjazz and removed request for kedz February 9, 2022 06:41

jupyterjazz suggested changes Feb 9, 2022

View reviewed changes

emysdias added 2 commits February 9, 2022 23:29

Merge branch 'main' of git://github.com/RasaHQ/rasa into remove_warn_…

26dd3f5

…and_correct_transformer_size

update files

d543349

jupyterjazz suggested changes Feb 10, 2022

View reviewed changes

emysdias added 7 commits February 19, 2022 16:54

remove line from changelog

00b3401

remove line from funct

24e4a52

Merge branch 'main' of git://github.com/RasaHQ/rasa into remove_warn_…

58cbbcd

…and_correct_transformer_size

change changelog

898f1ae

Merge branch 'main' of git://github.com/RasaHQ/rasa into remove_warn_…

cc2f1a6

…and_correct_transformer_size

add changelog

f299b5d

return some lines

b8ca0e4

tmbo requested a review from a team as a code owner September 12, 2022 19:48

tmbo force-pushed the main branch from 02dddfa to 00d369c Compare September 12, 2022 19:57

m-vdb closed this May 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove _warn_and_correct_transformer_size funct #10819

remove _warn_and_correct_transformer_size funct #10819

emysdias commented Feb 6, 2022 •

edited

Loading

CLAassistant commented Feb 6, 2022 •

edited

Loading

emysdias commented Feb 9, 2022

jupyterjazz left a comment •

edited

Loading

jupyterjazz Feb 9, 2022

emysdias Feb 10, 2022

emysdias Feb 10, 2022

jupyterjazz Feb 10, 2022

emysdias Feb 10, 2022

emysdias Feb 24, 2022

jupyterjazz Feb 9, 2022 •

edited

Loading

jupyterjazz Feb 10, 2022

jupyterjazz Feb 10, 2022

emysdias commented Feb 19, 2022

losterloh commented Jun 1, 2022

emysdias commented Jun 1, 2022

m-vdb commented May 25, 2023

		### Miscellaneous internal changes
		- [#10712](https://github.com/RasaHQ/rasa/issues/10712): remove _warn_and_correct_transformer_size function as it's not needed anymore

remove _warn_and_correct_transformer_size funct #10819

remove _warn_and_correct_transformer_size funct #10819

Conversation

emysdias commented Feb 6, 2022 • edited Loading

CLAassistant commented Feb 6, 2022 • edited Loading

emysdias commented Feb 9, 2022

jupyterjazz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jupyterjazz Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emysdias commented Feb 19, 2022

losterloh commented Jun 1, 2022

emysdias commented Jun 1, 2022

m-vdb commented May 25, 2023

emysdias commented Feb 6, 2022 •

edited

Loading

CLAassistant commented Feb 6, 2022 •

edited

Loading

jupyterjazz left a comment •

edited

Loading

jupyterjazz Feb 9, 2022 •

edited

Loading