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

warn for invalid keys for components #7134

Merged
merged 18 commits into from
Nov 5, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/6966.improvement.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Added warning for when an option is provided for a component that is not listed as a key in the defaults for that component.
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add the link to the documentation for Components please?

indam23 marked this conversation as resolved.
Show resolved Hide resolved
28 changes: 28 additions & 0 deletions rasa/nlu/components.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,34 @@ def validate_requirements(component_names: List[Optional[Text]]) -> None:
)


def validate_component_keys(pipeline: List["Component"]) -> None:
indam23 marked this conversation as resolved.
Show resolved Hide resolved
"""Validates that all keys for a component are valid.

Raises:
InvalidConfigError: If any component has a key specified that is not used
by the component class, it is likely a mistake in the pipeline

Args:
pipeline: The list of components in the piopeline
"""
from rasa.nlu import registry

for component in pipeline:
component_name = component.get("name")
component_class = registry.get_component_class(component_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this may raise an exception. Maybe it would be better to run the validation within _build_pipeline?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even better within component.create_component?

Copy link
Contributor

Choose a reason for hiding this comment

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

that would also avoid having to do the check in two locations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mm I tried moving it into component.create_component, but in that case extra keys are present that aren't allowed for the child class, but are for the base class (e.g. intent_classification is present for ResponseSelector). So, it isn't only checking the user-provided config at that point.

Re. putting it in _build_pipeline - you mean moving the logic inside the function instead of calling it from there? Which exception would that avoid?

Copy link
Contributor

Choose a reason for hiding this comment

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

it would have the advantage that you don't have to load the components yourself in _build_pipeline (which can go wrong and then you'd need error handling).You'd already have the instantiated components in _build_pipeline which should simplify your validations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved it - does that work?

allowed_keys = set(component_class.defaults.keys())
provided_keys = set(component.keys())
provided_keys.remove("name")
listseperator = "\n- "
indam23 marked this conversation as resolved.
Show resolved Hide resolved
for key in provided_keys:
if key not in allowed_keys:
rasa.shared.utils.io.raise_warning(
f"You have provided an invalid key `{key}` for component `{component_name}` in your pipeline. "
f"Valid options for `{component_name}` are:\n- "
f"{listseperator.join(allowed_keys)}"
indam23 marked this conversation as resolved.
Show resolved Hide resolved
)


def validate_empty_pipeline(pipeline: List["Component"]) -> None:
"""Ensures the pipeline is not empty.

Expand Down
2 changes: 2 additions & 0 deletions rasa/nlu/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ def __init__(
# required packages are available
if not self.skip_validation:
components.validate_requirements(cfg.component_names)
components.validate_component_keys(cfg.pipeline)

# build pipeline
self.pipeline = self._build_pipeline(cfg, component_builder)
Expand Down Expand Up @@ -338,6 +339,7 @@ def create(
# lets check if all required packages are available
if not skip_validation:
components.validate_requirements(model_metadata.component_classes)
components.validate_component_keys(model_metadata)

for i in range(model_metadata.number_of_components):
component_meta = model_metadata.for_component(i)
Expand Down
14 changes: 14 additions & 0 deletions tests/nlu/test_components.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,3 +200,17 @@ async def test_validate_requirements_raises_exception_on_component_without_name(
await train(
_config, data="./data/examples/rasa/demo-rasa.json", path=str(tmp_path),
)


async def test_validate_component_keys_raises_warning_on_invalid_key(tmp_path: Path,):
_config = RasaNLUModelConfig(
# config with a component that does not have a `name` property
indam23 marked this conversation as resolved.
Show resolved Hide resolved
{"pipeline": [{"name": "WhitespaceTokenizer", "confidence_threshold": 0.7}]}
wochinge marked this conversation as resolved.
Show resolved Hide resolved
)

with pytest.warns(UserWarning) as record:
await train(
_config, data="./data/examples/rasa/demo-rasa.json", path=str(tmp_path),
indam23 marked this conversation as resolved.
Show resolved Hide resolved
)

assert "You have provided an invalid key" in record[0].message.args[0]