Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove _warn_and_correct_transformer_size funct #10819

Closed
3 changes: 3 additions & 0 deletions CHANGELOG.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ https://github.com/RasaHQ/rasa/tree/main/changelog/ . -->
- [#10840](https://github.com/rasahq/rasa/issues/10840): Fix issue with missing running event loop in `MainThread` when starting Rasa Open
Source for Rasa X with JWT secrets.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't directly edit CHANGELOG.mdx. Here's one example of a changelog file:
Screenshot from 2022-02-10 18-12-12
Please follow the instructions given here and create a similar file.


## [3.0.6] - 2022-01-28
### Deprecations and Removals
Expand Down
23 changes: 1 addition & 22 deletions rasa/nlu/selectors/response_selector.py
Original file line number Diff line number Diff line change
Expand Up @@ -382,27 +382,6 @@ def _warn_about_transformer_and_hidden_layers_enabled(
category=UserWarning,
)

def _warn_and_correct_transformer_size(self, selector_name: Text) -> None:
"""Corrects transformer size so that training doesn't break; informs the user.

If a transformer is used, the default `transformer_size` breaks things.
We need to set a reasonable default value so that the model works fine.
"""
if (
self.component_config[TRANSFORMER_SIZE] is None
or self.component_config[TRANSFORMER_SIZE] < 1
):
rasa.shared.utils.io.raise_warning(
f"`{TRANSFORMER_SIZE}` is set to "
f"`{self.component_config[TRANSFORMER_SIZE]}` for "
f"{selector_name}, but a positive size is required when using "
f"`{NUM_TRANSFORMER_LAYERS} > 0`. {selector_name} will proceed, using "
f"`{TRANSFORMER_SIZE}={DEFAULT_TRANSFORMER_SIZE}`. "
f"Alternatively, specify a different value in the component's config.",
category=UserWarning,
)
self.component_config[TRANSFORMER_SIZE] = DEFAULT_TRANSFORMER_SIZE

def _check_config_params_when_transformer_enabled(self) -> None:
"""Checks & corrects config parameters when the transformer is enabled.

Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for your suggestion, but when I delete this line, I get 1 failed test
Captura de tela de 2022-02-09 23-37-26
t

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and 4 errors in def test_sets_integer_transformer_size_when_needed, like this:
Captura de tela de 2022-02-09 23-54-16
Captura de tela de 2022-02-09 23-54-04
Captura de tela de 2022-02-09 23-53-56
Captura de tela de 2022-02-09 23-37-26

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks for the suggestions!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 :(


def _check_config_parameters(self) -> None:
"""Checks that component configuration makes sense; corrects it where needed."""
Expand Down
10 changes: 10 additions & 0 deletions rasa/utils/tensorflow/rasa_layers.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
SENTENCE,
)
from rasa.utils.tensorflow import layers
import rasa.shared.utils.io
from rasa.utils.tensorflow.exceptions import TFLayerConfigException
from rasa.utils.tensorflow.transformer import TransformerEncoder
from rasa.nlu.constants import DEFAULT_TRANSFORMER_SIZE
Expand Down Expand Up @@ -810,6 +811,15 @@ def _get_transformer_dimensions(
if isinstance(transformer_units, dict):
transformer_units = transformer_units[attribute]
if transformer_layers > 0 and (not transformer_units or transformer_units < 1):
rasa.shared.utils.io.raise_warning(
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,
)
transformer_units = DEFAULT_TRANSFORMER_SIZE

return transformer_layers, transformer_units
Expand Down
6 changes: 0 additions & 6 deletions tests/nlu/selectors/test_selectors.py
Original file line number Diff line number Diff line change
Expand Up @@ -582,17 +582,11 @@ def test_sets_integer_transformer_size_when_needed(

if should_set_default_transformer_size:
assert len(records) > 0
# check that the specific warning was raised
assert any(warning_str in record.message.args[0] for record in records)
# check that transformer size got set to the new default
assert selector.component_config[TRANSFORMER_SIZE] == DEFAULT_TRANSFORMER_SIZE
else:
# check that the specific warning was not raised
assert not any(warning_str in record.message.args[0] for record in records)
# check that transformer size was not changed
assert selector.component_config[TRANSFORMER_SIZE] == config.get(
TRANSFORMER_SIZE, None # None is the default transformer size
)


def test_transformer_size_gets_corrected(train_persist_load_with_different_settings):
Expand Down