feat/OPM Pipeline #2
Conversation
extracts pipeline from ovos-core into a plugin, now tied to the ovos maintained adapt fork this opens the doors to continue integrating improvements such as the .excludes keyword
|
Warning Rate limit exceeded@JarbasAl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 49 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes introduce a new intent parsing service in Mycroft AI, enhancing multilingual support with the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
JarbasAl
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- ovos_adapt/opm.py (1 hunks)
- requirements.txt (1 hunks)
- setup.py (4 hunks)
Files skipped from review due to trivial changes (1)
- requirements.txt
Additional context used
Ruff
ovos_adapt/opm.py
180-180: Use of
functools.lru_cacheorfunctools.cacheon methods can lead to memory leaks(B019)
340-340: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
Additional comments not posted (17)
setup.py (2)
35-35: LGTM! Plugin entry point is correctly defined.The
PLUGIN_ENTRY_POINTis defined correctly for theovos-adapt-pipeline-plugin.
46-46: LGTM! Entry points updated correctly.The
entry_pointsparameter insetup()is updated correctly to include the newPLUGIN_ENTRY_POINT.ovos_adapt/opm.py (15)
67-77: LGTM! Deprecation handling is correct.The property
context_keywordsis correctly marked as deprecated, and appropriate warnings are logged when accessed.
79-88: LGTM! Deprecation handling is correct.The property
context_max_framesis correctly marked as deprecated, and appropriate warnings are logged when accessed.
90-98: LGTM! Deprecation handling is correct.The property
context_timeoutis correctly marked as deprecated, and appropriate warnings are logged when accessed.
100-109: LGTM! Deprecation handling is correct.The property
context_greedyis correctly marked as deprecated, and appropriate warnings are logged when accessed.
111-122: LGTM! Deprecation handling is correct.The property
context_manageris correctly marked as deprecated, and appropriate warnings are logged when accessed.
123-137: LGTM! Deprecation handling is correct.The method
update_contextis correctly marked as deprecated, and appropriate warnings are logged when called.
138-150: LGTM! High confidence intent matching logic is correct.The method
match_highcorrectly matches intents with high confidence.Verify confidence threshold.
Ensure that the confidence threshold
self.conf_highis appropriate for your use case.
152-164: LGTM! Medium confidence intent matching logic is correct.The method
match_mediumcorrectly matches intents with medium confidence.Verify confidence threshold.
Ensure that the confidence threshold
self.conf_medis appropriate for your use case.
166-178: LGTM! Low confidence intent matching logic is correct.The method
match_lowcorrectly matches intents with low confidence.Verify confidence threshold.
Ensure that the confidence threshold
self.conf_lowis appropriate for your use case.
259-267: LGTM! Deprecation handling is correct.The method
register_vocabis correctly marked as deprecated and callsregister_vocabularyinstead.
269-289: LGTM! Vocabulary registration logic is correct.The method
register_vocabularycorrectly registers skill vocabulary as adapt entities.Verify locking mechanism.
Ensure that the locking mechanism is appropriate and there are no potential race conditions.
290-298: LGTM! Intent registration logic is correct.The method
register_intentcorrectly registers new intents with the adapt engine.Verify locking mechanism.
Ensure that the locking mechanism is appropriate and there are no potential race conditions.
300-315: LGTM! Skill detachment logic is correct.The method
detach_skillcorrectly removes all intents for a skill and detaches related keywords and regexes.Verify locking mechanism.
Ensure that the locking mechanism is appropriate and there are no potential race conditions.
345-355: LGTM! Intent detachment logic is correct.The method
detach_intentcorrectly detaches a single intent.
357-360: LGTM! Shutdown logic is correct.The method
shutdowncorrectly drops all intent parsers for all languages.
| @lru_cache(maxsize=3) # NOTE - message is a string because of this | ||
| def match_intent(self, utterances: Tuple[str], | ||
| lang: Optional[str] = None, | ||
| message: Optional[str] = None): | ||
| """Run the Adapt engine to search for an matching intent. | ||
|
|
||
| Args: | ||
| utterances (iterable): utterances for consideration in intent | ||
| matching. As a practical matter, a single utterance will | ||
| be passed in most cases. But there are instances, such as | ||
| streaming STT that could pass multiple. Each utterance is | ||
| represented as a tuple containing the raw, normalized, and | ||
| possibly other variations of the utterance. | ||
| limit (float): confidence threshold for intent matching | ||
| lang (str): language to use for intent matching | ||
| message (Message): message to use for context | ||
|
|
||
| Returns: | ||
| Intent structure, or None if no match was found. | ||
| """ | ||
|
|
||
| if message: | ||
| message = Message.deserialize(message) | ||
| sess = SessionManager.get(message) | ||
|
|
||
| # we call flatten in case someone is sending the old style list of tuples | ||
| utterances = flatten_list(utterances) | ||
|
|
||
| utterances = [u for u in utterances if len(u.split()) < self.max_words] | ||
| if not utterances: | ||
| LOG.error(f"utterance exceeds max size of {self.max_words} words, skipping adapt match") | ||
| return None | ||
|
|
||
| lang = lang or self.lang | ||
| if lang not in self.engines: | ||
| return None | ||
|
|
||
| best_intent = {} | ||
|
|
||
| def take_best(intent, utt): | ||
| nonlocal best_intent | ||
| best = best_intent.get('confidence', 0.0) if best_intent else 0.0 | ||
| conf = intent.get('confidence', 0.0) | ||
| skill = intent['intent_type'].split(":")[0] | ||
| if best < conf and intent["intent_type"] not in sess.blacklisted_intents \ | ||
| and skill not in sess.blacklisted_skills: | ||
| best_intent = intent | ||
| # TODO - Shouldn't Adapt do this? | ||
| best_intent['utterance'] = utt | ||
|
|
||
| for utt in utterances: | ||
| try: | ||
| intents = [i for i in self.engines[lang].determine_intent( | ||
| utt, 100, | ||
| include_tags=True, | ||
| context_manager=sess.context)] | ||
| if intents: | ||
| utt_best = max( | ||
| intents, key=lambda x: x.get('confidence', 0.0) | ||
| ) | ||
| take_best(utt_best, utt) | ||
|
|
||
| except Exception as err: | ||
| LOG.exception(err) | ||
|
|
||
| if best_intent: | ||
| ents = [tag['entities'][0] for tag in best_intent['__tags__'] if 'entities' in tag] | ||
|
|
||
| sess.context.update_context(ents) | ||
|
|
||
| skill_id = best_intent['intent_type'].split(":")[0] | ||
| ret = IntentMatch( | ||
| 'Adapt', best_intent['intent_type'], best_intent, skill_id, | ||
| best_intent['utterance'] | ||
| ) | ||
| else: | ||
| ret = None | ||
| return ret | ||
|
|
There was a problem hiding this comment.
LGTM! Intent matching logic is correct.
The method match_intent correctly matches intents using the Adapt engine.
Potential memory leak issue.
The use of lru_cache on methods can lead to memory leaks. Consider using a different caching strategy.
Tools
Ruff
180-180: Use of
functools.lru_cacheorfunctools.cacheon methods can lead to memory leaks(B019)
extracts pipeline from ovos-core into a plugin, now tied to the ovos maintained adapt fork
this opens the doors to continue integrating improvements such as the .excludes keyword
needs OpenVoiceOS/ovos-plugin-manager#242 and OpenVoiceOS/ovos-plugin-manager#241
Summary by CodeRabbit
New Features
Chores
ovos-plugin-manager, ensuring compatibility with new features.