search

MycroftAI / adapt

Adapt Intent Parser
Apache License 2.0
712 stars 155 forks source link

Confusing examples: MultiIntent* examples define unused Parser and EntityTagger #109

Closed PLNech closed 3 years ago

PLNech commented 4 years ago

Problem

See the following excerpts of multi_intent_parser.py and multi_domain_intent_parser.py:

https://github.com/MycroftAI/adapt/blob/7c1e444cfbe1581ff97d4ef80752f8257a95f64c/examples/multi_intent_parser.py#L20-L25

https://github.com/MycroftAI/adapt/blob/7c1e444cfbe1581ff97d4ef80752f8257a95f64c/examples/multi_domain_intent_parser.py#L20-L25

In both cases, an EnglishTokenizer is imported, then used to create an EntityTagger and a Parser. But these two objects are not passed to the IntentDeterminationEngine, which actually only accepts a Tokenizer and Trie and always recreates its EntityTagger and Parser internally.

https://github.com/MycroftAI/adapt/blob/7c1e444cfbe1581ff97d4ef80752f8257a95f64c/adapt/engine.py#L39-L53

This makes understanding the different components of the Mycroft architecture more complicated that it should be.

Proposal

To clarify these examples, I believe they should only define objects that are useful to demonstrating what they achieve. Thus, I believe we should:

Alternatively, if there is a case for injecting an EntityTagger and a Parser when constructing the Engine, I believe that class should be refactored to allow using the objects currently defined but unused in these examples.

Discussion

Looking forward to hearing your take on this; should you agree with either proposal, happy to send a PR addressing this issue :smile:

clusterfudge commented 4 years ago

Hey @PLNech , thanks for your issue! You're right, it would appear that when I last updated the examples (around the same time much of the logic was encapsulated in IntentDeterminationEngine), I forgot to remove these instantiations from the class. You can see in this change where I updated the simpler samples correctly, but missed the multi-intent examples.I'll go back and clean those up, and tag you in the PR. Thanks again!

forslund commented 3 years ago

Fix merged in #110. Closing