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

Pass message metadata to interpreter when parsing #6275

Merged
merged 10 commits into from
Sep 22, 2020
Merged

Pass message metadata to interpreter when parsing #6275

merged 10 commits into from
Sep 22, 2020

Conversation

pheel
Copy link
Contributor

@pheel pheel commented Jul 24, 2020

Proposed changes:

  • Currently a NaturalLanguageInterpreter's parse method receives message text, id and the tracker. This adds an optional argument for the message's metadata as well, and thus allows metadata to affect interpretation.

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)

@sara-tagger
Copy link
Collaborator

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

@pheel
Copy link
Contributor Author

pheel commented Sep 8, 2020

@federicotdn Would you mind having a look at this one? 🙏

@federicotdn
Copy link
Contributor

Hey @pheel, could you share a bit more context on why you need the metadata for NaturalLanguageInterpreter.parse? Are you trying to do this on a custom subclass?

@pheel
Copy link
Contributor Author

pheel commented Sep 10, 2020

@federicotdn Exactly right! this is for a custom class, which makes use of contextual metadata sent over a socket to guide interpretation.

Right now an incoming message m's metadata is written into the tracker after interpretation, at the same time as the parse data that results from interpretation. Since NaturalLanguageInterpreter is given access to the pre-interpretation tracker state, containing all information about messages that came before m, I think it should have access to at least as much information about m itself.

Copy link
Contributor

@federicotdn federicotdn left a comment

Choose a reason for hiding this comment

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

Ok! Looks good. You you mind merging master into it? I think there might be some merge conflicts.

changelog/6275.feature.rst Outdated Show resolved Hide resolved
@federicotdn federicotdn self-assigned this Sep 11, 2020
Copy link
Contributor

@federicotdn federicotdn left a comment

Choose a reason for hiding this comment

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

Thanks!

@federicotdn federicotdn merged commit 1917a6c into RasaHQ:master Sep 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.

3 participants