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.

Sign up for GitHub

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

markers / validation #10012

Merged
merged 14 commits into from
Nov 2, 2021
Merged

markers / validation #10012

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
3404d26
validation; name vs tag; specify expected number of sub-markers;...
ka-bu Oct 29, 2021
2c31a5e
Delete old Marker test fixtures and associated files.
aeshky Oct 29, 2021
41a8ad4
Fix tests after rebasing main
usc-m Nov 1, 2021
95f4c83
First pass on domain validation
usc-m Nov 1, 2021
a98e985
Add some tests around domain validation
usc-m Nov 1, 2021
4231a41
Remove unused markers test data
usc-m Nov 1, 2021
3e3ce24
Config validation tests
usc-m Nov 1, 2021
ea3b0b3
Fix lint
usc-m Nov 1, 2021
f33b8c4
add tests for from_path; simplify other tests
ka-bu Nov 2, 2021
9a7ef5e
make domain path optional for _run_markers
ka-bu Nov 2, 2021
32e3a4f
Codeclimate fixes
usc-m Nov 2, 2021
663d651
Codeclimate fixes
usc-m Nov 2, 2021
c3d972e
Merge branch 'main' into markers/validate
usc-m Nov 2, 2021
730689a
Swap over warnings for errors
usc-m Nov 2, 2021
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
161 changes: 0 additions & 161 deletions data/test_markers/extracted_markers.json
View file
Open in desktop

This file was deleted.

15 changes: 14 additions & 1 deletion rasa/cli/evaluate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import argparse
from rasa.shared.core.domain import Domain
from typing import List, Text, Optional

from rasa.core.utils import AvailableEndpoints
Expand Down Expand Up @@ -87,6 +88,7 @@ def _run_markers_cli(args: argparse.Namespace) -> None:
seed,
count,
args.endpoints,
args.domain,
args.strategy,
args.config,
args.output_filename,
Expand All @@ -98,6 +100,7 @@ def _run_markers(
seed: Optional[int],
count: Optional[int],
endpoint_config: Text,
domain_path: Text,
strategy: Text,
config: Text,
output_filename: Text,
Expand All @@ -112,6 +115,8 @@ def _run_markers(
except 'all').
endpoint_config: Path to the endpoint configuration defining the tracker
store to use.
domain_path: Path to the domain specification to use when validating the
marker definitions.
strategy: Strategy to use when selecting trackers to extract from.
config: Path to the markers definition file to use.
output_filename: Path to write out the extracted markers.
Expand All @@ -128,8 +133,16 @@ def _run_markers(
"A file with the stats filename already exists"
)

tracker_loader = _create_tracker_loader(endpoint_config, strategy, count, seed)
domain = Domain.load(domain_path)
markers = Marker.from_path(config)

if not markers.validate_against_domain(domain):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice idea to just "bubble up" the warnings and then decide to break it here to gather all warnings first

Copy link
Contributor

Choose a reason for hiding this comment

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

I was hoping to do it more like a compiler where you can track line number and also collect warnings a bit more elegantly, but it seemed like adding that would involve a lot of complicated stuff around adding information about where markers were parsed from to the markers and I didn't want to do too much in the time we have left

rasa.shared.utils.cli.print_error_and_exit(
"Validation errors were found in the markers definition. "
"Please see errors listed above and fix before running again."
)

tracker_loader = _create_tracker_loader(endpoint_config, strategy, count, seed)
markers.export_markers(tracker_loader.load(), output_filename, stats_file)


Expand Down
47 changes: 47 additions & 0 deletions rasa/core/evaluation/marker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
from rasa.shared.core.domain import Domain
from typing import Optional, Text, List
from rasa.core.evaluation.marker_base import (
OperatorMarker,
ConditionMarker,
MarkerRegistry,
)
from rasa.shared.core.events import ActionExecuted, SlotSet, UserUttered, Event
import rasa.shared.utils.io


@MarkerRegistry.configurable_marker
Expand Down Expand Up @@ -147,6 +149,21 @@ def negated_tag() -> Optional[Text]:
"""Returns the tag to be used in a config file for the negated version."""
return "action_not_executed"

def validate_against_domain(self, domain: Domain) -> bool:
"""Checks that this marker (and its children) refer to entries in the domain.

Args:
domain: The domain to check against
"""
valid = self.text in domain.action_names_or_texts

if not valid:
rasa.shared.utils.io.raise_warning(
f"Referenced action '{self.text}' does not exist in the domain"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

@usc-m and @ka-bu I was creating the QA tests and was curious why this (and the same for intent and slot) is a warning and not an error. Does this mean that the extraction process will continue if there is a mismatch with the domain? What's the advantage of allowing this?

Copy link
Contributor

Choose a reason for hiding this comment

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

The extraction process isn't managed here - this goes over each marker and decides if they are valid, and if not emits a message, and then passes back True/False if it's valid or not. In the CLI we catch that boolean and error and exit based on it, but in these I wanted a way to collect all of the issues without stopping first, and then stop later (since reporting one at a time would be frustrating for users). The reason it's a warning is mostly because that was my first pass on it. Ideally it would go out as an error without terminating (and then get caught in the CLI to wrap it up and actually terminate) but we don't have a helper function for that so it's a little more involved - definitely something I want to fix before this merges.

Copy link
Contributor

Choose a reason for hiding this comment

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

The CLI also terminates before the extraction process begins. It loads the marker definitions and the domain, does the validation first, and terminates if issues are found. It will only establish a connection to the tracker store (and by extension perform extraction) if there are no issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or in short: #10012 (comment) :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Looked at how DialogueStateTracker manages this and have replicated

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 like that it's consistent but that approach is also a bit confusing, isn't it? I would've expected a "raise..." if there's a logger error (and usually, I would not explicitly log the error but just have a decorator to pass on the error message)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... on the other hand, getting an error as a result of "only warnings" is also a bit unexpected maybe 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I figured it was consistent from the perspective of the user, but it might be worth re-evaluating how we do error management across the codebase at some point


return valid

def _non_negated_version_applies_at(self, event: Event) -> bool:
return isinstance(event, ActionExecuted) and event.action_name == self.text

Expand All @@ -170,6 +187,21 @@ def negated_tag() -> Optional[Text]:
"""Returns the tag to be used in a config file for the negated version."""
return "intent_not_detected"

def validate_against_domain(self, domain: Domain) -> bool:
"""Checks that this marker (and its children) refer to entries in the domain.

Args:
domain: The domain to check against
"""
valid = self.text in domain.intent_properties

if not valid:
rasa.shared.utils.io.raise_warning(
f"Referenced intent '{self.text}' does not exist in the domain"
)

return valid

def _non_negated_version_applies_at(self, event: Event) -> bool:
return isinstance(event, UserUttered) and self.text in [
event.intent_name,
Expand All @@ -194,6 +226,21 @@ def negated_tag() -> Optional[Text]:
"""Returns the tag to be used in a config file for the negated version."""
return "slot_is_not_set"

def validate_against_domain(self, domain: Domain) -> bool:
"""Checks that this marker (and its children) refer to entries in the domain.

Args:
domain: The domain to check against
"""
valid = any(self.text == slot.name for slot in domain.slots)

if not valid:
rasa.shared.utils.io.raise_warning(
f"Referenced slot '{self.text}' does not exist in the domain"
)

return valid

def _non_negated_version_applies_at(self, event: Event) -> bool:
if isinstance(event, SlotSet) and event.key == self.text:
# slot is set if and only if it's value is not `None`
Expand Down
20 changes: 20 additions & 0 deletions rasa/core/evaluation/marker_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from __future__ import annotations
import os
from abc import ABC, abstractmethod
from rasa.shared.core.domain import Domain
from rasa.shared.core.trackers import DialogueStateTracker
from rasa.utils.io import WriteRow
from typing import (
Expand Down Expand Up @@ -267,6 +268,15 @@ def __len__(self) -> int:
"""Returns the count of all markers that are part of this marker."""
...

@abstractmethod
def validate_against_domain(self, domain: Domain) -> bool:
"""Checks that this marker (and its children) refer to entries in the domain.

Args:
domain: The domain to check against
"""
...

def evaluate_events(
self, events: List[Event], recursive: bool = False
) -> List[Dict[Text, List[EventMetaData]]]:
Expand Down Expand Up @@ -727,6 +737,16 @@ def reset(self) -> None:
marker.reset()
super().reset()

def validate_against_domain(self, domain: Domain) -> bool:
"""Checks that this marker (and its children) refer to entries in the domain.

Args:
domain: The domain to check against
"""
return all(
marker.validate_against_domain(domain) for marker in self.sub_markers
)

@staticmethod
def from_tag_and_sub_config(
tag: Text, sub_config: Any, name: Optional[Text] = None,
Expand Down
10 changes: 0 additions & 10 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,16 +168,6 @@ def endpoints_path() -> Text:
return "data/test_endpoints/example_endpoints.yml"


@pytest.fixture(scope="session")
def extracted_markers_json() -> Text:
return "data/test_markers/extracted_markers.json"


@pytest.fixture(scope="session")
def marker_stats_output_json() -> Text:
return "stats_output.json"


# https://github.com/pytest-dev/pytest-asyncio/issues/68
# this event_loop is used by pytest-asyncio, and redefining it
# is currently the only way of changing the scope of this fixture
Expand Down
Loading