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

OSS-668: Rasa Knowledge base actions are unable to query about a certain attribute of an object unless the user first asks to obtain a list of objects of a specific type. #922

Merged
merged 175 commits into from
Jun 6, 2023

Conversation

Thirunayan22
Copy link
Contributor

@Thirunayan22 Thirunayan22 commented Jan 10, 2023

Proposed changes:

  • The improvement requires changes to the classes ActionQueryKnowledgeBase and InMemoryKnowledgeBase under rasa-sdk.
  • The object_type can be inferred by utilizing the entity classification (DIET) where object types are used as entities to annotate object names.
  • This also requires changes to be made to slot management to enable dynamic inference of object_type.
  • E.g: If the user asks ‘price range of Berlin Burrito Company’, then rasa will extract and set attribute slot value to ‘price-range’ and hotel slot value to ‘Berlin Burrito Company’. From this it can be inferred that the user is talking about object type ‘hotel’.

Status (please check what you already did):

  • made PR ready for code review
  • added some tests for the functionality
  • updated the documentation in the rasaHQ/rasa
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@CLAassistant
Copy link

CLAassistant commented Jan 10, 2023

CLA assistant check
All committers have signed the CLA.

Gajithra and others added 4 commits January 17, 2023 09:00
Pulling updates from RasaHQ/rasa-sdk main branch to rootcodelabs/rasa-sdk OSS-668 branch
@ancalita ancalita self-requested a review February 1, 2023 15:56
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.

Many thanks for your contribution 💯
I have left some suggestions and questions about implementation choices in this PR. Please troubleshoot the failing unit tests as can be seen in the CI runs (for example 3.10 job run) and also add unit tests for the functionality you've added. You can run the tests locally too through your IDE or running make test in a venv where you've installed rasa-sdk.

changelog/668.bugfix.md Outdated Show resolved Hide resolved
rasa_sdk/knowledge_base/utils.py Outdated Show resolved Hide resolved
rasa_sdk/knowledge_base/utils.py Outdated Show resolved Hide resolved
rasa_sdk/knowledge_base/utils.py Outdated Show resolved Hide resolved
rasa_sdk/knowledge_base/utils.py Outdated Show resolved Hide resolved
rasa_sdk/knowledge_base/actions.py Outdated Show resolved Hide resolved
rasa_sdk/knowledge_base/actions.py Show resolved Hide resolved
rasa_sdk/knowledge_base/actions.py Show resolved Hide resolved
rasa_sdk/knowledge_base/actions.py Show resolved Hide resolved
rasa_sdk/knowledge_base/actions.py Outdated Show resolved Hide resolved
@Gajithra
Copy link
Contributor

Gajithra commented Feb 2, 2023

Thanks @ancalita for the feedback! We will work on it to make the changes.

Gajithra and others added 19 commits February 2, 2023 23:23
Pulling updates from RasaHQ/rasa-sdk main to rootcodelabs/rasa-sdk OSS-668
RAOC-5: added comprehensive typing hint
RAOC-10: Uncommented code snippet that was previously commented
@Gajithra
Copy link
Contributor

Gajithra commented May 22, 2023

Thank you for your patience throughout this reviewing process and for addressing all my questions and comments 💯 ! I left some fresh comments, I will proceed to reviewing the docs PR now and I will also test with the public repo you provided. In the interest of time, I first shared these initial comments and I will follow-up if I have any other comments after manual testing.

Thanks for sharing your initial comments @ancalita 🙌🏽 . We have addressed the requested changes here and in the docs PR in rasa repo.

Pulling updates from RasaHQ/rasa-sdk main to rootcodelabs/rasa-sdk OSS-668
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.

Looks good 💯 ! Very close to getting merged as well as soon as the last few comments on the unit test are addressed 👍🏻

tests/knowledge_base/test_actions.py Outdated Show resolved Hide resolved
tests/knowledge_base/test_actions.py Outdated Show resolved Hide resolved
tests/knowledge_base/test_actions.py Outdated Show resolved Hide resolved
@Gajithra
Copy link
Contributor

Gajithra commented Jun 5, 2023

Looks good 💯 ! Very close to getting merged as well as soon as the last few comments on the unit test are addressed 👍🏻

Great to hear that @ancalita 🤞🏽! We have addressed your comments and made the requested changes.

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.

Nice one 🎉 Thank you for all your hard work 🥇

@ancalita ancalita merged commit 2e32306 into RasaHQ:main Jun 6, 2023
@RasaHQ RasaHQ deleted a comment from sp-rasa Jun 13, 2023
@sonam-pankaj95
Copy link

Congratulations @Gajithra and @Thirunayan22 🎉, would love to see you in contributor's program as well. Thank you @ancalita for leading this 💜

Sorry this is my official account 👍

@sonam-pankaj95
Copy link

Hey @Thirunayan22 and @Gajithra ,

Can you please share your email ids, here or mail them at s.pankaj@rasa.com, So that I can add both of you in our contributor's program and send some swags.

Thanks,
Sonam

@Thirunayan22
Copy link
Contributor Author

Hi @sonam-pankaj95 , here are our email ids. I have sent the email addresses to you in email too.

Thirunayan: thirunayan.dinesh@rootcode.io
Gajithira: gajithira.puvanendran@rootcode.io

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.

6 participants