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

Various changes #12408

Closed
wants to merge 574 commits into from
Closed

Various changes #12408

wants to merge 574 commits into from

Conversation

tmbo
Copy link
Member

@tmbo tmbo commented May 17, 2023

Proposed changes:

  • added functionality to send a message from a policy using an action. the policy sets the text as metadata for the action. the action will use that text to send an utterance back to the user.

Feel free to fork of this branch to add additional functionality. Let's try to keep the history of this branch clean, as in one commit per feature. Please make sure that tests are passing for the merged changes.

TODO:

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)

@tmbo tmbo requested a review from a team as a code owner May 17, 2023 12:24
@tmbo tmbo removed the request for review from a team May 17, 2023 12:26
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

Dropping by just as a curiosity, left two passing comments 😄 👍🏻

Comment on lines 198 to 223
### End-to-End stories

As part of the beta, we're also releasing a beta version of a new End-To-End
testing framework. The `rasa test e2e` command allows you to test your bot
end-to-end, i.e. from the user's perspective. You can use it to test your bot in
a variety of ways, including testing the `IntentlessPolicy`.

To use the new testing framework, you need to define a set of test cases in a
test folder, e.g. `tests/e2e_test_stories.yml`. The test cases are defined in a
similar format as stories are, but contain the user's messages and the bot's
responses. Here's an example:

```yaml title="tests/e2e_test_stories.yml"
test_cases:
- test_case: transfer charge
steps:
- user: how can I send money without getting charged?
- utter: utter_faq_0
- user: not zelle. a normal transfer
- utter: utter_faq_7
```

**Please ensure all your test stories have unique names!** After setting the
beta feature flag for E2E testing in your current shell with
`export RASA_PRO_BETA_E2E=true`, you can run the tests with
`rasa test e2e -f tests/e2e_test_stories.yml`
Copy link
Member

Choose a reason for hiding this comment

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

We should remove this section since there is a different part of the docs dedicated to end-to-end testing.

@@ -719,12 +714,18 @@ async def retrieve_tracker(request: Request, conversation_id: Text) -> HTTPRespo
"""Get a dump of a conversation's tracker including its events."""
verbosity = event_verbosity_parameter(request, EventVerbosity.AFTER_RESTART)
until_time = rasa.utils.endpoints.float_arg(request, "until")

tracker = await app.ctx.agent.processor.fetch_full_tracker_with_initial_session(
Copy link
Member

Choose a reason for hiding this comment

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

Why was fetch_full_tracker_with_initial_session removed? I added this method in order to fix a bug reported for the HTTP API 🤔 Only concerned that zombie bugs would reappear.

Copy link
Member Author

Choose a reason for hiding this comment

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

it is still there, but there is a paraemeter that allows you not to do it. if this is always done, this creates a new tracker in cases you are fetching a non existant conversation and sometimes you don't want to create a tracker for that

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes I see the start_session parameter, but in the else branch get_tracker is used which in turn uses tracker_store.get_or_create_tracker while fetch_full_tracker was using tracker_store.get_or_create_full_tracker 🤔

@tmbo
Copy link
Member Author

tmbo commented Jun 19, 2023

Dropping by just as a curiosity, left two passing comments 😄 👍🏻

thanks a lot for stopping by 🙂 🙏

Copy link
Collaborator

Choose a reason for hiding this comment

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

this file is not under tests/, is this intended?

Copy link
Collaborator

Choose a reason for hiding this comment

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

this file is not under the root tests/ folder, is this intended?

@@ -20,6 +20,10 @@ responses:
utter_did_that_help:
- text: "Did that help you?"

utter_flow_continue_interrupted:
Copy link
Member Author

Choose a reason for hiding this comment

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

@twerkmeister should this be here given that the same is in the default flows file?

@tmbo tmbo changed the base branch from main to llm-labs June 26, 2023 13:20
Base automatically changed from llm-labs to main July 6, 2023 11:52
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

I had a look through the Flows modules primarily and left a few exploratory questions ahead of the call tomorrow morning 🙏🏻

logger = logging.getLogger(__name__)


class FlowTriggerAction(action.Action):
Copy link
Member

Choose a reason for hiding this comment

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

Is this action executing the entire of a Flow or only a Flow Step?

flow_action_name: Name of the flow.
"""
super().__init__()
self._flow_name = flow_action_name[len(FLOW_PREFIX) :]
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that a prefix gets added to all flow names defined in yaml?

]

events: List[Event] = [
SlotSet(FLOW_STACK_SLOT, stack.as_dict())
Copy link
Member

Choose a reason for hiding this comment

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

How is this default slot used? Can it get updated with every new flow that gets predicted? What happens to this slot when flows are interlinked?

rasa/core/policies/flow_policy.py Show resolved Hide resolved
Comment on lines 164 to 167
predicted_action = None

# if detector predicted an action, we don't want to predict a flow
if predicted_action is not None:
Copy link
Member

Choose a reason for hiding this comment

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

When could predicted_action be something else but None?



@dataclass
class Flow:
Copy link
Member

Choose a reason for hiding this comment

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

I've seen mentioned meta-flows: what are these and do these come out of the box if flows are enabled, or do users need to write them according to prescriptive instructions?

available_steps = {step.id for step in self.steps}
for step in self.steps:
for link in step.next.links:
if link.target not in available_steps:
Copy link
Member

Choose a reason for hiding this comment

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

What is a link target?

self.slots.append(
AnySlot(flow_slot, mappings=[], influence_conversation=False)
)
else:
Copy link
Member

Choose a reason for hiding this comment

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

When would this occur? If it's overridden in the domain.yml already?

Here are some examples of conditions that demonstrate the use of different constructs.

```yaml
# Simple conditions
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we should move this to Writing Conditions section.

Urkem and others added 27 commits October 16, 2023 09:45
…builtin slots (#12917)

* filter out dialogue stack slot set events from custom actions, mark builtin slots, add unit tests

* add comment
moved existing examples into subfolder
…mmandGenerator

Eng 472 component wrap up llm command generator
…appings

Tutorial template remove slot mappings
* implement changes + unit test

* handle edge case
* Remove EntryPromptFlowStep class.

* remove entry_prompt from yaml schema
* Catch KeyError when running the graph.

* increase minimum compatibile version to 3.7.0b3

* update model version in test

* use constant for minimal compatible version

* add changelog

* remove changelog as it is not needed

* Update error message.
* moved imports of langchain

* improve imports
…tion-refactor-tests-review

wrap up and tests for flow trigger action
…t values - [ENG 607] (#12942)

* modify prompt to include possible categorical slot values
@tmbo tmbo closed this Oct 30, 2023
@vdanyliv
Copy link

@tmbo could you please clarify why this pr is closed? are you still planning to merge flows?

@tmbo tmbo deleted the dm2 branch November 22, 2023 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.