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

Metada on session start action #5611

Merged
merged 23 commits into from
Apr 22, 2020
Merged

Metada on session start action #5611

merged 23 commits into from
Apr 22, 2020

Conversation

lluchini
Copy link
Contributor

@lluchini lluchini commented Apr 9, 2020

Proposed changes:
Closes #5574

  • The default action 'action_session_start' wasn't able to receive the metadata from the message sent, RASA when initiating a new session it sends a blank tracker with no information whatsoever, ignoring the metadata into the message.

Adjusted to add the event sessionStart to tracker events and with it the metadata from the message.

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)

able to receive the metadata from the message sent,
RASA when initiating a new session it sends a blank
tracker with no information whatsoever, ignoring
the metadata into the message.

Adjusted to add the event sessionStart to tracker
events and with it the metadata from the message.
Method:
get_tracker_with_session_start

Parameter:
metadata
@CLAassistant
Copy link

CLAassistant commented Apr 9, 2020

CLA assistant check
All committers have signed the CLA.

@lluchini lluchini changed the title Metada on session start action Metada on session start action Closes #5574 Apr 9, 2020
@lluchini
Copy link
Contributor Author

lluchini commented Apr 9, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

lluchini seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Done!

@lluchini lluchini changed the title Metada on session start action Closes #5574 Metada on session start action Apr 9, 2020
@sara-tagger
Copy link
Collaborator

Thanks for submitting a pull request 🚀 @degiz will take a look at it as soon as possible ✨

@sara-tagger sara-tagger requested a review from degiz April 10, 2020 06:00
@degiz degiz added this to the Rasa 1.10 milestone Apr 16, 2020
Copy link
Contributor

@degiz degiz left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR 🚀

Left several comments. Looking forward to have this code merged! 👍

tests/core/test_actions.py Outdated Show resolved Hide resolved
rasa/core/processor.py Outdated Show resolved Hide resolved
@lluchini lluchini requested a review from degiz April 16, 2020 20:24
Copy link
Contributor

@degiz degiz left a comment

Choose a reason for hiding this comment

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

I didn't get what you meant by more "high level".

The most high level (in terms of structure) code we've modified in this PR is get_tracker_with_session_start(). So I'd expect the unit tests that covers this change to call this method with metadata and assert on the results.

modified get_tracker_with_session_start().
So the unit tests that covers this change to call
this method with metadata and assert on the results.
@lluchini
Copy link
Contributor Author

lluchini commented Apr 17, 2020

I didn't get what you meant by more "high level".

The most high level (in terms of structure) code we've modified in this PR is get_tracker_with_session_start(). So I'd expect the unit tests that cover this change to call this method with metadata and assert the results.

I hope that I had understand what you meant, I added a test into test_processor.py to this specific method and assertion of the metadata content.

@lluchini
Copy link
Contributor Author

The deep source is failing with a false positive.

@lluchini
Copy link
Contributor Author

Faling Not My Fault :)

Continuous Integration / Build Docker (docker/Dockerfile_pretrained_embeddings_spacy_en, -spacy-en) (pull_request) Failing after 7m — Build Docker (docker/Dockerfile_pretrained_embeddings_spacy_en, -spacy-en)

Motive

Error processing tar file(exit status 1): write /root/.cache/pip/http/d/9/c/c/b/d9ccb85290d9889b7fd067a45d9cd98ce7bcd4ee2d66f167f5e940c2: no space left on device

@degiz degiz self-requested a review April 17, 2020 13:07
Copy link
Contributor

@degiz degiz left a comment

Choose a reason for hiding this comment

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

Awesome test, thanks!

Last comment, and we can merge it.

changelog/5574.bugfix.rst Outdated Show resolved Hide resolved
tests/core/test_actions.py Outdated Show resolved Hide resolved
tests/core/test_processor.py Outdated Show resolved Hide resolved
lluchini and others added 3 commits April 17, 2020 13:33
@lluchini lluchini requested a review from degiz April 17, 2020 19:19
@lluchini
Copy link
Contributor Author

  • DeepSource is a false positive;
  • Continuous Integration / Build Docker is a problem of space; and
  • Continuous Integration / Run Tests can't know, it doesn't say what's wrong and I'm not able to run tests on Windows (apparently Sanic doesn't like windows very much, I'm working on it...).

Copy link
Contributor

@degiz degiz left a comment

Choose a reason for hiding this comment

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

Great job! 👍
I'm approving the PR, let's make sure the unit tests are passing in the CI.

@lluchini
Copy link
Contributor Author

lluchini commented Apr 21, 2020

@degiz

Great job! 👍
I'm approving the PR, let's make sure the unit tests are passing in the CI.

Thanks, you made my first contribution to an open-source project so smooth, I really appreciate!

So is that it? do I have to do something more?

@degiz
Copy link
Contributor

degiz commented Apr 21, 2020

@lluchini welcome to the club 🚀

Now we're basically trying to merge the PR before the master goes ahead 😄

@degiz degiz merged commit 002b892 into RasaHQ:master Apr 22, 2020
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.

metadata missing in action_session_start
4 participants