flow tag in import and export #1887
Conversation
WalkthroughThe changes introduce a new static method for rule validation in the Changes
Sequence Diagram(s)sequenceDiagram
participant YAMLLoader
participant TrainingDataValidator
participant Validator
TrainingDataValidator->>YAMLLoader: Load schema from rules.yml
YAMLLoader-->>TrainingDataValidator: Return schema
TrainingDataValidator->>YAMLLoader: Load rules from provided file
YAMLLoader-->>TrainingDataValidator: Return rules
TrainingDataValidator->>Validator: Validate rules against schema
alt Validation errors exist
Validator-->>TrainingDataValidator: Return errors
TrainingDataValidator->>Caller: Raise ValueError
else No validation errors
Validator-->>TrainingDataValidator: Return success
TrainingDataValidator-->>Caller: Return validated rules
end
sequenceDiagram
participant TrainingDataValidator
participant KRasaFileImporter
participant ConfigLoader
TrainingDataValidator->>KRasaFileImporter: Call from_training_files(...)
KRasaFileImporter->>ConfigLoader: Load training/config files
ConfigLoader-->>KRasaFileImporter: Return file data
KRasaFileImporter-->>TrainingDataValidator: Return processed training data
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Definitions (1)tests/unit_test/validator/data_importer_test.py (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (16)
✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (9)
kairon/importer/validator/file_validator.py (1)
1115-1130: Added new validate_rules static methodThis method validates rules against a predefined schema, but has some potential improvements:
- The error handling should be consistent with the rest of the codebase (using AppException instead of ValueError)
- Missing docstring documentation
- Schema file path is hardcoded and not configurable
Consider enhancing the method with proper error handling and documentation:
@staticmethod def validate_rules(rules_path: str): + """ + Validates rules defined in a YAML file against a schema. + + Args: + rules_path: Path to the rules YAML file + + Raises: + AppException: If rules validation fails + """ current_dir = os.path.dirname(os.path.realpath(__file__)) schema_file_path = os.path.join(current_dir, "..", "shared", "schemas", "rules.yml") with open(schema_file_path, 'r') as schema_file: schema = yaml.safe_load(schema_file) validator = Validator(schema) with open(rules_path, 'r') as rules_file: rules = yaml.safe_load(rules_file) if not validator.validate(rules): - raise ValueError(f"Validation errors: {validator.errors}") + raise AppException(f"Rules validation failed: {validator.errors}")kairon/shared/data/model_data_imporer.py (5)
35-39: Provide a docstring for clarity.This constructor looks good overall, but it lacks a docstring. Adding one can improve maintainability by clarifying the purpose and usage of
CustomRuleStep.
41-47: Document the constructor parameters.While the implementation is straightforward, consider adding a docstring to describe the parameters and how
flow_tagsis used. It will make the class more comprehensible and maintainable.
81-90: Replace debug print with logging.Using a direct
- print("CustomStoryGraph created with flow_tags:", self.flow_tags) + import logging + logging.debug(f"CustomStoryGraph created with flow_tags: {self.flow_tags}")
91-133: Use “key in dict” instead of “key in dict.keys()”.To address the linter warnings and Python best practices, remove
.keys()altogether when checking membership for lines 110, 112, 114, 116, 118, 120, 122, and 124.- elif KEY_USER_INTENT in step.keys(): + elif KEY_USER_INTENT in step: - elif KEY_OR in step.keys(): + elif KEY_OR in step: ... (repeat for each occurrence) ...🧰 Tools
🪛 Ruff (0.8.2)
110-110: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
110-110: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
112-112: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
114-114: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
116-116: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
118-118: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
120-120: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
122-122: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
124-124: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
168-193: Remove unused variableflow_tags.The method
_extract_flow_tagsis never utilized after its return value is assigned at line 170, resulting in an unused variable.def get_stories(self, exclusion_percentage: Optional[int] = None) -> CustomStoryGraph: story_graph = self._get_stories(exclusion_percentage) - flow_tags = self._extract_flow_tags(story_graph) return CustomStoryGraph(story_graph.story_steps)🧰 Tools
🪛 Ruff (0.8.2)
170-170: Local variable
flow_tagsis assigned to but never usedRemove assignment to unused variable
flow_tags(F841)
kairon/shared/data/processor.py (3)
4989-4998: Consider optimizing the isinstance check.The current implementation uses multiple
isinstancecalls which can be merged for better readability and performance.def __extract_rules(self, story_steps, bot: str, user: str): saved_rules = self.fetch_rule_block_names(bot) for story_step in story_steps: if ( - (isinstance(story_step, CustomRuleStep) or isinstance(story_step, RuleStep)) + isinstance(story_step, (CustomRuleStep, RuleStep)) and story_step.block_name.strip().lower() not in saved_rules ): rule = self.__extract_rule_events(story_step, bot, user) yield rule🧰 Tools
🪛 Ruff (0.8.2)
4994-4994: Multiple
isinstancecalls forstory_step, merge into a single callMerge
isinstancecalls forstory_step(SIM101)
5092-5092: Small typo in return type annotation.There's a missing space between the colon and return type annotation.
- def get_rules_for_download(self, bot: Text) ->StoryGraph: + def get_rules_for_download(self, bot: Text) -> StoryGraph:
164-5118: Consider adding documentation for flow tag functionality.The implementation of flow tags looks good, but adding documentation comments to explain the purpose and behavior of flow tags would help future developers understand the feature better. Consider adding documentation to describe how flow tags work and the difference between "agentic_flow" and "chatbot_flow".
🧰 Tools
🪛 Ruff (0.8.2)
311-314: Return the condition
config.get("dynamic_url_slot_name") == slotdirectlyReplace with
return config.get("dynamic_url_slot_name") == slot(SIM103)
324-328: Use
return any(set_slot.get("name") == slot for set_slot in set_slots)instead offorloopReplace with
return any(set_slot.get("name") == slot for set_slot in set_slots)(SIM110)
338-341: Return the condition directly
Inline condition
(SIM103)
352-356: Use
return any(MongoProcessor.is_slot_in_field(note, slot) for note in notes)instead offorloopReplace with
return any(MongoProcessor.is_slot_in_field(note, slot) for note in notes)(SIM110)
361-364: Return the condition
set_slot == slotdirectlyReplace with
return set_slot == slot(SIM103)
378-382: Use
return any(set_slot.get("name") == slot for set_slot in set_slots)instead offorloopReplace with
return any(set_slot.get("name") == slot for set_slot in set_slots)(SIM110)
397-401: Use
return any(value == slot for value in metadata.values())instead offorloopReplace with
return any(value == slot for value in metadata.values())(SIM110)
410-413: Return the condition
config.get("set_slot") == slotdirectlyReplace with
return config.get("set_slot") == slot(SIM103)
418-421: Use
return any(set_slot.get("name") == slot for set_slot in set_slots)instead offorloopReplace with
return any(set_slot.get("name") == slot for set_slot in set_slots)(SIM110)
453-457: Use
return any(set_slot.get("name") == slot for set_slot in set_slots)instead offorloopReplace with
return any(set_slot.get("name") == slot for set_slot in set_slots)(SIM110)
498-498:
kairon.importer.validator.file_validator.TrainingDataValidatorimported but unusedRemove unused import:
kairon.importer.validator.file_validator.TrainingDataValidator(F401)
553-553: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
569-569: Do not perform function call
REQUIREMENTS.copyin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
679-682: Use ternary operator
type_value = "text" if not metadata else "json"instead ofif-else-blockReplace
if-else-block withtype_value = "text" if not metadata else "json"(SIM108)
725-725: Do not perform function call
REQUIREMENTS.copyin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
806-806: Local variable
actionsis assigned to but never usedRemove assignment to unused variable
actions(F841)
1274-1274: Use
bool(...)instead ofTrue if ... else FalseReplace with `bool(...)
(SIM210)
1347-1347: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
1384-1385: Use a single
ifstatement instead of nestedifstatements(SIM102)
1467-1468: Use a single
ifstatement instead of nestedifstatements(SIM102)
1525-1525: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
1528-1528: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
1790-1790: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
2181-2181: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
2348-2351: Use a single
ifstatement instead of nestedifstatements(SIM102)
2762-2762: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
2833-2833: Use
example.get("entities", None)instead of anifblockReplace with
example.get("entities", None)(SIM401)
2921-2921: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
2924-2924: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
2972-2972: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
3206-3206: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
3298-3302: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
3549-3549: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
3621-3621: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
3645-3645: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
3893-3893: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
3942-3942: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
3946-3949: Use
contextlib.suppress(AppException)instead oftry-except-passReplace with
contextlib.suppress(AppException)(SIM105)
3997-3997: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
4096-4098: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
4100-4101: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
4113-4113: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
4136-4136: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
4159-4159: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
4192-4192: Local variable
responseis assigned to but never usedRemove assignment to unused variable
response(F841)
4311-4313: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
4527-4527: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
4587-4587: Use
key not in dictinstead ofkey not in dict.keys()Remove
.keys()(SIM118)
4714-4714: Use
action_config.get("website")instead ofaction_config.get("website", None)Replace
action_config.get("website", None)withaction_config.get("website")(SIM910)
4755-4755: Use
action_config.get("website")instead ofaction_config.get("website", None)Replace
action_config.get("website", None)withaction_config.get("website")(SIM910)
4917-4917: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
4955-4955: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
4994-4994: Multiple
isinstancecalls forstory_step, merge into a single callMerge
isinstancecalls forstory_step(SIM101)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
kairon/importer/validator/file_validator.py(4 hunks)kairon/shared/data/model_data_imporer.py(1 hunks)kairon/shared/data/processor.py(8 hunks)kairon/shared/utils.py(3 hunks)tests/unit_test/data_processor/data_processor_test.py(2 hunks)tests/unit_test/events/events_test.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
tests/unit_test/events/events_test.py (1)
kairon/shared/data/model_data_imporer.py (1)
KRasaFileImporter(167-199)
kairon/shared/data/model_data_imporer.py (1)
kairon/shared/data/processor.py (1)
get_stories(3652-3763)
kairon/shared/utils.py (1)
kairon/shared/data/model_data_imporer.py (2)
KYAMLStoryWriter(202-284)dump(262-284)
kairon/shared/data/processor.py (3)
kairon/shared/data/model_data_imporer.py (2)
KRasaFileImporter(167-199)CustomRuleStep(35-38)kairon/shared/models.py (1)
FlowTagType(127-129)kairon/shared/data/data_objects.py (1)
Rules(716-748)
🪛 Ruff (0.8.2)
kairon/shared/data/model_data_imporer.py
110-110: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
110-110: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
112-112: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
114-114: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
116-116: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
118-118: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
120-120: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
122-122: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
124-124: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
170-170: Local variable flow_tags is assigned to but never used
Remove assignment to unused variable flow_tags
(F841)
kairon/shared/data/processor.py
4994-4994: Multiple isinstance calls for story_step, merge into a single call
Merge isinstance calls for story_step
(SIM101)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python CI
- GitHub Check: Analyze (python)
🔇 Additional comments (20)
tests/unit_test/events/events_test.py (2)
23-23: Import change looks goodThe change from using
RasaFileImporterto usingKRasaFileImporteraligns with the PR objectives for supporting flow tag in import and export.
76-78: Implementation correctly uses the new importer classThe test fixture now correctly uses
KRasaFileImporterinstead ofRasaFileImporterwhile maintaining the same method signature. This should allow the tests to properly handle the new flow tag functionality in stories and rules.tests/unit_test/data_processor/data_processor_test.py (2)
5281-5281: Additional metadata structure successfully implemented for rules.The assertion now includes a metadata section with flow_tags for rules, which is consistent with the changes introduced to support flow tags in the data model.
5296-5296:❓ Verification inconclusive
Verify if all rules should have metadata with flow_tags.
While the first rule includes metadata with flow_tags, the second rule (
multiflow_story_story_download_data_files_1) doesn't have any metadata section. Please verify if this inconsistency is intentional or if all rules should have flow tags.
🏁 Script executed:
#!/bin/bash # Check for consistency of metadata structure across rule definitions in other test files # Look for patterns of rule definitions with and without metadata echo "Rules with metadata:" rg -A 3 "rule:" --glob "tests/**/*.py" | rg -A 3 "metadata:" echo "Rules without metadata that might need updating:" rg -A 5 "rule:" --glob "tests/**/*.py" | rg -v "metadata:" | rg "steps:"Length of output: 3955
Verify consistency in metadata inclusion for rule definitions.
In
tests/unit_test/data_processor/data_processor_test.py, the rule"rule for download"includes a metadata section withflow_tags, whereas the rulemultiflow_story_story_download_data_files_1does not include any metadata. The grep results also show that some tests (like those inutility_test.py) define rules without metadata, suggesting an inconsistency.
- Location:
tests/unit_test/data_processor/data_processor_test.py(line 5296) for the rule in question.- Observation: Mixed patterns across the codebase where some rules include metadata (
flow_tags) and others do not.- Action: Please verify whether the omission of the metadata section for the rule
multiflow_story_story_download_data_files_1is intentional. If all rules are meant to include metadata withflow_tags, update the definitions accordingly to ensure consistency.kairon/shared/utils.py (3)
86-86: New import for supporting flow tags in rule stepsThis import is part of a broader change to support flow tags, with the
KYAMLStoryWriterclass handling the serialization of story steps that include flow tag metadata.
1206-1206: Changed YAMLStoryWriter to KYAMLStoryWriter for writing rule stepsThe new
KYAMLStoryWriteris now used to write rule steps, which will preserve flow tag metadata when dumping rules to YAML format.
1635-1635: Added type hints to method parametersAdding type hints improves code clarity and enables better IDE support and static type checking.
kairon/importer/validator/file_validator.py (3)
18-18: Added ruamel.yaml importImport of ruamel.yaml library is needed for the new rule validation functionality.
30-30: New import for KRasaFileImporterThis import enables the use of the custom file importer that supports flow tags in import and export operations.
81-83: Replaced RasaFileImporter with KRasaFileImporterThe custom KRasaFileImporter class is now used instead of the standard RasaFileImporter to support flow tag metadata during import.
kairon/shared/data/model_data_imporer.py (4)
48-62: Verify single-step return logic.
_next_story_stepscurrently returns a single-step list, which may be correct if each rule or story block is meant to contain exactly one step. Otherwise, this might limit multi-step usage. Review the call sites to verify that we only ever need a single step here.
65-78: Class initialization looks consistent.The usage of
CustomStoryStepBuilderto incorporateflow_tagsin rule parsing is well-structured. No issues found here.
134-137: Confirm overwriting of existing flow tags.
_parse_metadataassignsflow_tagstoself.current_step_builder.flow_tags, overwriting any previously set tags. If you require merging or preserving earlier tags, add the relevant logic here.
201-289: Overall structure appears sound.The YAML writer correctly processes rule steps, ensuring flow tags are serialized under
metadata. This aligns well with the custom story/rule parsing approach.kairon/shared/data/processor.py (6)
164-164: Good addition of new classes for enhanced functionality.The import now includes
KRasaFileImporterandCustomRuleStepfrom the custom importer module, which will enable flow tag support.
233-233: Well implemented switch to use the flow tag-aware method.You've correctly updated to use
get_rules_for_download()instead of the previousget_rules_for_training()when downloading files, ensuring flow tags are properly included.
5001-5025: Good implementation of flow tag support in rule events extraction.This method has been properly updated to support flow tags:
- Type annotation now correctly accepts either
CustomRuleSteporRuleStep- Default flow tag is set to
[FlowTagType.chatbot_flow.value]- Flow tags are conditionally extracted from the rule step when available
- The flow tags are properly passed to the Rules object
5089-5091: Good addition of return type annotation.Adding the
-> StoryGraphreturn type annotation enhances type safety and code clarity.
5092-5118: Well implemented method for downloading rules with flow tags.This new method appropriately:
- Creates
CustomRuleStepobjects that include flow tags for each rule- Transfers all rule properties including flow tags
- Returns a
StoryGraphwith the complete rule informationNote that while
get_rules_for_training()only includes rules with the chatbot flow tag, this method includes all rules with their respective flow tags, which makes it ideal for export/download functionality.
4801-4801: Good improvement of method signature with stronger typing.Adding proper type annotations for
bot: stranduser: strimproves code clarity and type safety.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
kairon/shared/data/model_data_imporer.py (8)
81-81: Remove debug print statement before productionThis print statement should be removed or converted to a proper logging statement in production code.
- print("CustomStoryGraph created with flow_tags:", self.flow_tags) + # Use logger instead if debugging information is needed + # logger.debug(f"CustomStoryGraph created with flow_tags: {self.flow_tags}")
102-116: Optimize dictionary key lookupsUsing
key in dict.keys()is less efficient than directly usingkey in dict. This pattern appears multiple times in the code.- elif KEY_USER_INTENT in step.keys() or KEY_USER_MESSAGE in step.keys(): + elif KEY_USER_INTENT in step or KEY_USER_MESSAGE in step: self._parse_user_utterance(step) - elif KEY_OR in step.keys(): + elif KEY_OR in step: self._parse_or_statement(step) - elif KEY_ACTION in step.keys(): + elif KEY_ACTION in step: self._parse_action(step) - elif KEY_BOT_END_TO_END_MESSAGE in step.keys(): + elif KEY_BOT_END_TO_END_MESSAGE in step: self._parse_bot_message(step) - elif KEY_CHECKPOINT in step.keys(): + elif KEY_CHECKPOINT in step: self._parse_checkpoint(step) - elif KEY_SLOT_NAME in step.keys(): + elif KEY_SLOT_NAME in step: self._parse_slot(step) - elif KEY_ACTIVE_LOOP in step.keys(): + elif KEY_ACTIVE_LOOP in step: self._parse_active_loop(step[KEY_ACTIVE_LOOP]) - elif KEY_METADATA in step.keys(): + elif KEY_METADATA in step: self._parse_metadata(step)🧰 Tools
🪛 Ruff (0.8.2)
102-102: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
102-102: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
104-104: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
106-106: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
108-108: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
110-110: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
112-112: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
114-114: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
116-116: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
191-194: Consider documenting the purpose of the custom YAML representerThe custom YAML representer for lists is added but its purpose is not documented. Consider adding a comment explaining why this custom representation is needed.
self.yaml = YAML() + # Custom representer to ensure lists are properly formatted in the YAML output self.yaml.representer.add_representer(list, self.represent_list) def represent_list(self, dumper, data): return dumper.represent_sequence('tag:yaml.org,2002:seq', data)
1-2: File has a typo in its nameThe filename contains a typo: "imporer" should be "importer". Consider renaming the file and updating all imports accordingly.
This typo should be fixed to maintain code clarity and consistency.
35-36: Inconsistent argument formatting in CustomStoryStepBuilderThe
flow_tagsparameter has a default value but doesn't use type hints, unlike the parent class constructor. Consider adding type hints for consistency.def __init__( - self, flow_tags = None, *args, **kwargs + self, flow_tags: Optional[List[str]] = None, *args, **kwargs ) -> None:
67-67: Missing type hint for flow_tags parameterThe
flow_tagsparameter is missing a type hint. For consistency with the rest of the code, add the appropriate type hint.- def _new_rule_part(self, name: Text, source_name: Optional[Text], flow_tags = None) -> None: + def _new_rule_part(self, name: Text, source_name: Optional[Text], flow_tags: Optional[List[str]] = None) -> None:
242-243: Consider handling potential type conversion issuesThe code converts flow tags to strings using
str(tag)which might not handle complex objects gracefully. Consider adding validation or error handling for unexpected tag types.if hasattr(rule_step, "flow_tags"): - result["metadata"] = {"flow_tags": [str(tag) for tag in rule_step.flow_tags]} + try: + result["metadata"] = {"flow_tags": [str(tag) for tag in rule_step.flow_tags]} + except Exception as e: + # Log the error and use a default or skip the problematic tag + result["metadata"] = {"flow_tags": [str(tag) for tag in rule_step.flow_tags if isinstance(tag, (str, int, float, bool))]}
159-168: Consider adding docstrings to the KRasaFileImporter classThe
KRasaFileImporterclass and its methods lack docstrings. Consider adding documentation to explain the purpose and behavior of these methods.class KRasaFileImporter(RasaFileImporter): + """Custom implementation of RasaFileImporter that creates and returns CustomStoryGraph objects.""" + def get_stories(self, exclusion_percentage: Optional[int] = None) -> CustomStoryGraph: + """Override the get_stories method to return a CustomStoryGraph instead of StoryGraph. + + Args: + exclusion_percentage: Optional percentage of stories to exclude + + Returns: + CustomStoryGraph object containing parsed story steps with flow tags + """ story_graph = self._get_stories(exclusion_percentage) return CustomStoryGraph(story_graph.story_steps) def _get_stories(self, exclusion_percentage: Optional[int] = None) -> CustomStoryGraph: + """Load stories from files and create a CustomStoryGraph. + + Args: + exclusion_percentage: Optional percentage of stories to exclude + + Returns: + CustomStoryGraph object containing parsed story steps with flow tags + """ return CustomStoryGraph(self.load_data_from_files( self._story_files, self.get_domain(), exclusion_percentage ))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
kairon/shared/data/model_data_imporer.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
kairon/shared/data/model_data_imporer.py (1)
kairon/shared/data/processor.py (1)
get_stories(3652-3763)
🪛 Ruff (0.8.2)
kairon/shared/data/model_data_imporer.py
102-102: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
102-102: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
104-104: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
106-106: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
108-108: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
110-110: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
112-112: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
114-114: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
116-116: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (python)
- GitHub Check: Python CI
* flow tag in import and export * flow tag in import and export * flow tag in import and export * flow tag in import and export * flow tag in import and export --------- Co-authored-by: spandan_mondal <spandan.mondal@nimblework.com>
Summary by CodeRabbit
New Features
Enhancements
Tests