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

Disallow dialogue stack slot from being set in custom actions & mark builtin slots #12917

Merged
Show file tree
Hide file tree
Changes from all 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
8 changes: 8 additions & 0 deletions rasa/core/actions/action.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
)
from rasa.shared.core import events
from rasa.shared.core.constants import (
DIALOGUE_STACK_SLOT,
USER_INTENT_OUT_OF_SCOPE,
ACTION_LISTEN_NAME,
ACTION_RESTART_NAME,
Expand Down Expand Up @@ -829,6 +830,13 @@ async def run(
)

evts = events.deserialise_events(events_json)
# filter out `SlotSet` events for internal `dialogue_stack` slot
evts = [
event
for event in evts
if not (isinstance(event, SlotSet) and event.key == DIALOGUE_STACK_SLOT)
]

return cast(List[Event], bot_messages) + evts

except ClientResponseError as e:
Expand Down
21 changes: 18 additions & 3 deletions rasa/shared/core/domain.py
Original file line number Diff line number Diff line change
Expand Up @@ -993,7 +993,12 @@ def _add_flow_slots(self) -> None:
for flow_slot in FLOW_SLOT_NAMES:
if flow_slot not in slot_names:
self.slots.append(
AnySlot(flow_slot, mappings=[], influence_conversation=False)
AnySlot(
flow_slot,
mappings=[],
influence_conversation=False,
is_builtin=True,
)
)
else:
# TODO: figure out what to do here.
Expand All @@ -1016,6 +1021,7 @@ def _add_requested_slot(self) -> None:
rasa.shared.core.constants.REQUESTED_SLOT,
mappings=[],
influence_conversation=False,
is_builtin=True,
)
)

Expand All @@ -1041,12 +1047,21 @@ def _add_knowledge_base_slots(self) -> None:
for slot in KNOWLEDGE_BASE_SLOT_NAMES:
if slot not in slot_names:
self.slots.append(
TextSlot(slot, mappings=[], influence_conversation=False)
TextSlot(
slot,
mappings=[],
influence_conversation=False,
is_builtin=True,
)
)

def _add_session_metadata_slot(self) -> None:
self.slots.append(
AnySlot(rasa.shared.core.constants.SESSION_START_METADATA_SLOT, mappings=[])
AnySlot(
rasa.shared.core.constants.SESSION_START_METADATA_SLOT,
mappings=[],
is_builtin=True,
)
)

def index_for_action(self, action_name: Text) -> int:
Expand Down
29 changes: 26 additions & 3 deletions rasa/shared/core/slots.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ def __init__(
initial_value: Any = None,
value_reset_delay: Optional[int] = None,
influence_conversation: bool = True,
is_builtin: bool = False,
) -> None:
"""Create a Slot.

Expand All @@ -46,6 +47,9 @@ def __init__(
initial_value. This is behavior is currently not implemented.
influence_conversation: If `True` the slot will be featurized and hence
influence the predictions of the dialogue polices.
is_builtin: `True` if the slot is a built-in slot, `False` otherwise.
Built-in slots also encompass user writable slots (via custom actions),
such as `return_value`.
"""
self.name = name
self.mappings = mappings
Expand All @@ -54,6 +58,7 @@ def __init__(
self._value_reset_delay = value_reset_delay
self.influence_conversation = influence_conversation
self._has_been_set = False
self.is_builtin = is_builtin

def feature_dimensionality(self) -> int:
"""How many features this single slot creates.
Expand Down Expand Up @@ -180,6 +185,7 @@ def __init__(
max_value: float = 1.0,
min_value: float = 0.0,
influence_conversation: bool = True,
is_builtin: bool = False,
) -> None:
"""Creates a FloatSlot.

Expand All @@ -188,7 +194,12 @@ def __init__(
UserWarning, if initial_value is outside the min-max range.
"""
super().__init__(
name, mappings, initial_value, value_reset_delay, influence_conversation
name,
mappings,
initial_value,
value_reset_delay,
influence_conversation,
is_builtin,
)
self.max_value = max_value
self.min_value = min_value
Expand Down Expand Up @@ -318,10 +329,16 @@ def __init__(
initial_value: Any = None,
value_reset_delay: Optional[int] = None,
influence_conversation: bool = True,
is_builtin: bool = False,
) -> None:
"""Creates a `Categorical Slot` (see parent class for detailed docstring)."""
super().__init__(
name, mappings, initial_value, value_reset_delay, influence_conversation
name,
mappings,
initial_value,
value_reset_delay,
influence_conversation,
is_builtin,
)
if values and None in values:
rasa.shared.utils.io.raise_warning(
Expand Down Expand Up @@ -417,6 +434,7 @@ def __init__(
initial_value: Any = None,
value_reset_delay: Optional[int] = None,
influence_conversation: bool = False,
is_builtin: bool = False,
) -> None:
"""Creates an `Any Slot` (see parent class for detailed docstring).

Expand All @@ -433,7 +451,12 @@ def __init__(
)

super().__init__(
name, mappings, initial_value, value_reset_delay, influence_conversation
name,
mappings,
initial_value,
value_reset_delay,
influence_conversation,
is_builtin,
)

def __eq__(self, other: Any) -> bool:
Expand Down
22 changes: 22 additions & 0 deletions tests/core/test_actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -3039,3 +3039,25 @@ def test_default_actions_and_names_consistency():
RULE_SNIPPET_ACTION_NAME
}
assert names_of_default_actions == names_of_executable_actions_in_constants


async def test_filter_out_dialogue_stack_slot_set_in_a_custom_action(
default_channel: OutputChannel,
default_nlg: NaturalLanguageGenerator,
default_tracker: DialogueStateTracker,
domain: Domain,
) -> None:
endpoint = EndpointConfig("https://example.com/webhooks/actions")
remote_action = action.RemoteAction("my_action", endpoint)
events = [SlotSet(DIALOGUE_STACK_SLOT, {}), SlotSet("some_slot", "some_value")]
events_as_dict = [event.as_dict() for event in events]
response = {"events": events_as_dict, "responses": []}
with aioresponses() as mocked:
mocked.post("https://example.com/webhooks/actions", payload=response)

events = await remote_action.run(
default_channel, default_nlg, default_tracker, domain
)

assert len(events) == 1
assert events[0] == SlotSet("some_slot", "some_value")
10 changes: 10 additions & 0 deletions tests/shared/core/test_domain.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
from rasa.shared.core.slots import InvalidSlotTypeException, TextSlot
from rasa.shared.core.constants import (
DEFAULT_INTENTS,
KNOWLEDGE_BASE_SLOT_NAMES,
SLOT_LISTED_ITEMS,
SLOT_LAST_OBJECT,
SLOT_LAST_OBJECT_TYPE,
Expand Down Expand Up @@ -2371,3 +2372,12 @@ def test_domain_with_slots_without_mappings(caplog: LogCaptureFixture) -> None:
"Slot 'slot_without_mappings' has no mappings defined. "
"We will continue with an empty list of mappings."
) in caplog.text


def test_domain_default_slots_are_marked_as_builtin(domain: Domain) -> None:
all_default_slot_names = DEFAULT_SLOT_NAMES.union(KNOWLEDGE_BASE_SLOT_NAMES)
domain_default_slots = [
slot for slot in domain.slots if slot.name in all_default_slot_names
]

assert all(slot.is_builtin for slot in domain_default_slots)
4 changes: 4 additions & 0 deletions tests/shared/core/test_slots.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,10 @@ def test_slot_fingerprint_uniqueness(
f2 = slot.fingerprint()
assert f1 != f2

def test_slot_is_not_builtin_by_default(self, mappings: List[Dict[Text, Any]]):
slot = self.create_slot(mappings, influence_conversation=False)
assert not slot.is_builtin


class TestTextSlot(SlotTestCollection):
def create_slot(
Expand Down
Loading