feat/domain_engine #31
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Suggested labels
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 using PR comments)
Other keywords and placeholders
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: 4
🧹 Outside diff range and nitpick comments (4)
ovos_padatious/domain_engine.py (2)
128-131: Avoid reusing variable names to prevent confusionIn the
calc_intentmethod, the parameterdomainis reassigned within the method usingdomain = domain or self.domain_engine.calc_intent(query).name. Reusing the variable namedomaincan lead to confusion and reduce code readability. Consider using a different variable name for the local assignment, such asresolved_domain.Apply this diff to rename the local variable:
- domain: str = domain or self.domain_engine.calc_intent(query).name + resolved_domain = domain or self.domain_engine.calc_intent(query).name + if resolved_domain in self.domains: + return self.domains[resolved_domain].calc_intent(query) - if domain in self.domains: - return self.domains[domain].calc_intent(query)
147-154: Clarify variable naming in loops to enhance readabilityIn the
calc_intentsmethod, the parameterdomainand the loop variabledomainrepresent different entities—one is a string, and the other is aMatchDataobject. This reuse of the variable namedomainmay cause confusion. Consider renaming the loop variable to something likedomain_matchfor clarity.Apply this diff to rename the loop variable:
matches = [] domains = self.calc_domains(query)[:top_k_domains] - for domain in domains: - if domain.name in self.domains: - matches += self.domains[domain.name].calc_intents(query) + for domain_match in domains: + if domain_match.name in self.domains: + matches += self.domains[domain_match.name].calc_intents(query)tests/test_domain.py (1)
5-5: Remove placeholder comment in import statementLine 5 contains a placeholder comment that suggests replacing
'your_module'with the actual module name. Since the module is correctly imported, the comment can be removed to clean up the code.Apply this diff to remove the comment:
- from ovos_padatious.domain_engine import DomainIntentEngine # Replace 'your_module' with the actual module name + from ovos_padatious.domain_engine import DomainIntentEngineovos_padatious/intent_container.py (1)
59-59: Update docstring to reflect optionalcache_dirparameterSince
cache_diris now optional and defaults to a computed path if not provided, the docstring should be updated to indicate thatcache_diris optional.Apply this diff to update the docstring:
""" Creates an IntentContainer object used to load and match intents Args: - cache_dir (str): Directory for caching the neural network models and intent/entity files. + cache_dir (Optional[str]): Directory for caching the neural network models and intent/entity files. If not provided, a default path is used. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
ovos_padatious/domain_engine.py(1 hunks)ovos_padatious/intent_container.py(3 hunks)ovos_padatious/intent_manager.py(1 hunks)tests/test_domain.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
tests/test_domain.py
3-3: ovos_padatious.intent_container.IntentContainer imported but unused
Remove unused import: ovos_padatious.intent_container.IntentContainer
(F401)
🔇 Additional comments (2)
ovos_padatious/intent_manager.py (1)
25-28: Addition of intent_names property is appropriate
The new intent_names property correctly provides a list of intent names from both trained intents and intents pending training. This enhances accessibility and aligns with best practices.
ovos_padatious/intent_container.py (1)
70-73: The addition of intent_names property enhances encapsulation
The new intent_names property provides external access to the intent names managed by the IntentContainer, promoting encapsulation and improving usability.
match the skill first, and then the intent
match the skill first, and then the intent
JarbasAl
force-pushed
the
feat/domain_engine
branch
from
d15f923 to
582e12d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
ovos_padatious/domain_container.py (1)
135-135: Avoid reassigning function parameters with type annotationsIn line 135, the parameter
domainis reassigned with a type annotation:domain: str = domain or self.domain_engine.calc_intent(query).nameReassigning a function parameter can lead to confusion and is generally discouraged. Additionally, re-annotating the variable
domainis unnecessary since it is already typed in the function signature. Consider assigning the value to a new local variable without re-annotation:-domain: str = domain or self.domain_engine.calc_intent(query).name +current_domain = domain or self.domain_engine.calc_intent(query).nametests/test_domain.py (1)
4-4: Remove placeholder comment in import statementIn line 4, there is a placeholder comment that is no longer needed:
from ovos_padatious.domain_container import DomainIntentContainer # Replace 'your_module' with the actual module nameSince the module import is already correct, remove the placeholder comment to clean up the code:
-from ovos_padatious.domain_container import DomainIntentContainer # Replace 'your_module' with the actual module name +from ovos_padatious.domain_container import DomainIntentContainer
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ovos_padatious/domain_container.py(1 hunks)ovos_padatious/intent_container.py(3 hunks)ovos_padatious/intent_manager.py(1 hunks)ovos_padatious/opm.py(11 hunks)tests/test_domain.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ovos_padatious/intent_manager.py
- ovos_padatious/intent_container.py
| skill_id = message.data.get("skill_id") or message.context.get("skill_id") | ||
| self._skill2intent[skill_id].append(message.data['name']) | ||
|
|
There was a problem hiding this comment.
Ensure skill_id is properly obtained and handled
In the register_intent method, skill_id is obtained using:
skill_id = message.data.get("skill_id") or message.context.get("skill_id")If skill_id is None, the intent might be registered under a None key in _skill2intent, which could lead to issues when detaching intents or skills. It's important to ensure that skill_id is not None. Consider adding a check to handle this situation:
if not skill_id:
LOG.error("Skill ID is missing. Cannot register intent without a valid skill ID.")
returnThis will prevent unintended behaviors and ensure that each intent is associated with a valid skill.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
ovos_padatious/opm.py (1)
117-127: Consider adding validation for engine_class parameterWhile the implementation is correct, it would be beneficial to validate that the provided engine_class is a valid PadatiousEngine type.
self.engine_class = engine_class or IntentContainer +if not issubclass(self.engine_class, (IntentContainer, DomainIntentContainer)): + raise ValueError(f"Invalid engine_class: {engine_class}. Must be IntentContainer or DomainIntentContainer")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ovos_padatious/opm.py(11 hunks)
🔇 Additional comments (7)
ovos_padatious/opm.py (7)
36-36: Correct the import statement for DomainIntentContainer
The import statement is incorrect as noted in previous reviews.
-from ovos_padatious.domain_container import DomainIntentContainer
+from ovos_padatious.domain_engine import DomainIntentContainer41-43: LGTM! Well-defined type alias.
The PadatiousEngine type alias improves code readability and maintainability by clearly defining the allowed engine types.
Line range hint 98-116: LGTM! Well-implemented engine class selection.
The implementation provides flexibility in engine selection while maintaining backward compatibility through config.
241-246: LGTM! Proper handling of intent removal for both container types.
The implementation correctly differentiates between container types and uses the appropriate removal method.
Line range hint 274-292: LGTM! Well-structured registration logic.
The implementation properly handles both container types and correctly manages skill_id.
304-306: Ensure skill_id is properly obtained and handled
The skill_id handling could be improved to prevent None values.
skill_id = message.data.get("skill_id") or message.context.get("skill_id")
+if not skill_id:
+ LOG.error("Skill ID is missing. Cannot register intent without a valid skill ID.")
+ return
self._skill2intent[skill_id].append(message.data['name'])422-422: LGTM! Accurate type annotation.
The type annotation correctly represents the possible container types.
match the skill first, and then the intent
Summary by CodeRabbit
New Features
DomainIntentContainerclass for domain-aware intent recognition.intent_namesproperty to bothIntentContainerandIntentManagerclasses for easier access to intent names.PadatiousPipelineto accept different intent container types.Bug Fixes
Tests
DomainIntentContainerto ensure correct intent registration and calculations.Chores