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

Merge resposnes from NLU and Domain when there're no retrieval intents #7390

Merged
merged 6 commits into from Dec 4, 2020

Conversation

degiz
Copy link
Contributor

@degiz degiz commented Nov 26, 2020

Proposed changes:

  • Make sure the responses are synced between NLU data and the Domain even if there're no retrieval intents in the training data.

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)

@degiz degiz requested a review from a team as a code owner November 26, 2020 18:58
@degiz degiz requested review from Tawakalt and removed request for a team and Tawakalt November 26, 2020 18:58
@degiz degiz changed the base branch from master to 2.1.x November 26, 2020 18:59
@degiz degiz force-pushed the fix_sync_of_responses branch 2 times, most recently from 6ae3b4f to 5a0011c Compare November 26, 2020 19:02
@@ -65,7 +65,6 @@ async def get_domain(self) -> Domain:
return domain
try:
domain = Domain.load(self._domain_path)
domain.check_missing_templates()
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this check be done somewhere if not here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added it here

Copy link
Contributor

@dakshvar22 dakshvar22 left a comment

Choose a reason for hiding this comment

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

Question: Using the RetrievalModelDataImporter to achieve this without having any retrieval intents seems weird. Shouldn't the logic of syncing all responses defined in NLU data(irrespective of them being response templates for retrieval intents) be done inside CombinedDataImporter itself and then RetrievalModelDataImporter adds retrieval intents to domain and fetches the response templates from domain. What do you think?

@degiz
Copy link
Contributor Author

degiz commented Nov 30, 2020

@dakshvar22 I agree, that RetrievalModelDataImporter seems like a non appropriate place. However, since the "RetrievalModelDataImporter" is used literally always and importing of retrieval intents involves syncing the domain and NLU, do you think it makes sense to rename it instead?
I don't really have a strong opinion, both moving the sync into a CombinedDataImporter and renaming will work for me.

@dakshvar22
Copy link
Contributor

@degiz I also do not have very strong opinions but renaming it more appropriately(something like - NLUDomainSyncImporter) seems like a better approach I think.

@degiz degiz requested a review from dakshvar22 December 1, 2020 14:35
@degiz
Copy link
Contributor Author

degiz commented Dec 2, 2020

@dakshvar22 I've change the name! Do you have time to re-review?

Copy link
Contributor

@dakshvar22 dakshvar22 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. Thanks for fixing this 💯

@degiz degiz merged commit 2f8d4c2 into 2.1.x Dec 4, 2020
@degiz degiz deleted the fix_sync_of_responses branch December 4, 2020 09:25
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.

None yet

2 participants