feat/opm_pipeline_plugin #22
Conversation
move pipeline from core into plugin
|
Warning Rate limit exceeded@JarbasAl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 39 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 update introduces the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PadaciosoPipeline
participant MessageBus
participant IntentMatcher
User->>PadaciosoPipeline: Send utterance
PadaciosoPipeline->>MessageBus: Broadcast intent matching request
MessageBus->>IntentMatcher: Evaluate utterances
IntentMatcher-->>MessageBus: Return matched intent
MessageBus-->>PadaciosoPipeline: Provide matched intent
PadaciosoPipeline-->>User: Respond with intent details
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (3)
setup.py (1)
50-50: DefinePLUGIN_ENTRY_POINTat the top of the file.For better readability and maintainability, consider defining
PLUGIN_ENTRY_POINTat the top of the file with other constants.+ PLUGIN_ENTRY_POINT = 'ovos-padacioso-pipeline-plugin=ovos_padacioso.opm:PadaciosoPipeline'padacioso/opm.py (2)
1-1: Add module-level docstring.Consider adding a module-level docstring to provide an overview of the module's purpose and functionality.
+ """ + This module provides the PadaciosoPipeline class and related functionality for intent matching. + """
227-232: Clarify docstring.The docstring mentions compatibility with Padacioso twice, which seems redundant. Consider rephrasing it for clarity.
- Note that this method is NOT compatible with Padacioso, but is compatible with Padacioso. + Note that this method is NOT compatible with Padatious, but is compatible with Padacioso.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- padacioso/opm.py (1 hunks)
- setup.py (2 hunks)
Additional context used
Ruff
padacioso/opm.py
191-191: Ambiguous variable name:
l(E741)
Additional comments not posted (1)
setup.py (1)
63-63: LGTM!The entry point is correctly added to the
setup()function.
| # TODO - how to disambiguate ? | ||
| intent = ties[0] |
There was a problem hiding this comment.
TODO: Add disambiguation logic.
The TODO comment suggests adding logic for disambiguation. Consider implementing this or creating an issue to track this task.
Do you want me to generate the disambiguation logic or open a GitHub issue to track this task?
There was a problem hiding this comment.
@JarbasAl, I'll add the disambiguation logic to the _calc_padacioso_intent function. Here's the updated implementation:
@lru_cache(maxsize=3) # repeat calls under different conf levels wont re-run code
def _calc_padacioso_intent(utt: str,
intent_container: FallbackIntentContainer,
sess: Session) -> \
Optional[PadaciosoIntent]:
"""
Try to match an utterance to an intent in an intent_container
@param args: tuple of (utterance, IntentContainer)
@return: matched PadaciosoIntent
"""
try:
intents = [i for i in intent_container.calc_intents(utt)
if i is not None
and i["name"] not in sess.blacklisted_intents
and i["name"].split(":")[0] not in sess.blacklisted_skills]
if len(intents) == 0:
return None
best_conf = max(x.get("conf", 0) for x in intents if x.get("name"))
ties = [i for i in intents if i.get("conf", 0) == best_conf]
if not ties:
return None
# Disambiguation logic: select the intent with the shortest matched phrase
intent = min(ties, key=lambda i: len(i.get("matches", {}).get("utterance", "")))
return PadaciosoIntent(intent["name"], utt, intent["matches"], intent["conf"])
except Exception as e:
LOG.error(f"Error calculating Padacioso intent: {e}")
return NoneThis logic resolves ties by selecting the intent with the shortest matched phrase. If you have any other criteria in mind for disambiguation, please let me know!
Learnings added
Learnt from: JarbasAl
PR: OpenVoiceOS/padacioso#22
File: padacioso/opm.py:284-285
Timestamp: 2024-07-20T17:59:52.692Z
Learning: When multiple intents have the same confidence score, disambiguation logic should be implemented to select the best intent. One approach is to select the intent with the shortest matched phrase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
it should prefer the longest matched phrase, not the shortest
There was a problem hiding this comment.
@JarbasAl, I'll update the disambiguation logic to prefer the longest matched phrase. Here's the revised implementation:
@lru_cache(maxsize=3) # repeat calls under different conf levels wont re-run code
def _calc_padacioso_intent(utt: str,
intent_container: FallbackIntentContainer,
sess: Session) -> \
Optional[PadaciosoIntent]:
"""
Try to match an utterance to an intent in an intent_container
@param args: tuple of (utterance, IntentContainer)
@return: matched PadaciosoIntent
"""
try:
intents = [i for i in intent_container.calc_intents(utt)
if i is not None
and i["name"] not in sess.blacklisted_intents
and i["name"].split(":")[0] not in sess.blacklisted_skills]
if len(intents) == 0:
return None
best_conf = max(x.get("conf", 0) for x in intents if x.get("name"))
ties = [i for i in intents if i.get("conf", 0) == best_conf]
if not ties:
return None
# Disambiguation logic: select the intent with the longest matched phrase
intent = max(ties, key=lambda i: len(i.get("matches", {}).get("utterance", "")))
return PadaciosoIntent(intent["name"], utt, intent["matches"], intent["conf"])
except Exception as e:
LOG.error(f"Error calculating Padacioso intent: {e}")
return NoneThis logic now resolves ties by selecting the intent with the longest matched phrase. Let me know if further adjustments are needed!
Learnings added
Learnt from: JarbasAl
PR: OpenVoiceOS/padacioso#22
File: padacioso/opm.py:284-285
Timestamp: 2024-07-20T18:03:35.671Z
Learning: When multiple intents have the same confidence score, disambiguation logic should be implemented to select the best intent. One approach is to select the intent with the longest matched phrase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Learnings used
Learnt from: JarbasAl
PR: OpenVoiceOS/padacioso#22
File: padacioso/opm.py:284-285
Timestamp: 2024-07-20T17:59:52.848Z
Learning: When multiple intents have the same confidence score, disambiguation logic should be implemented to select the best intent. One approach is to select the intent with the shortest matched phrase.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
move pipeline from core into plugin
Summary by CodeRabbit
New Features
Bug Fixes
Documentation