Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce a new module Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
KaQuMiQ
changed the title
KaQuMiQ
force-pushed
the
feature/metaprompt
branch
from
cc47f4a to
f54458e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/draive/__init__.py(3 hunks)src/draive/helpers/__init__.py(2 hunks)src/draive/helpers/instruction_preparation.py(1 hunks)src/draive/instructions/__init__.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/draive/helpers/__init__.py (1)
src/draive/helpers/instruction_preparation.py (1)
prepare_instruction(16-52)
src/draive/__init__.py (2)
src/draive/helpers/instruction_preparation.py (1)
prepare_instruction(16-52)src/draive/instructions/types.py (2)
Instruction(50-211)InstructionDeclaration(42-46)
src/draive/instructions/__init__.py (1)
src/draive/instructions/types.py (1)
InstructionDeclaration(42-46)
🔇 Additional comments (2)
src/draive/helpers/__init__.py (1)
1-1: Public-API update looks good
prepare_instructionis correctly imported and re-exported via__all__.
No circular-import risk is introduced becauseinstruction_preparationhas no dependency ondraive.helpers.
Nice & tidy change.Also applies to: 16-16
src/draive/instructions/__init__.py (1)
5-6: LGTM – symmetric export
InstructionDeclarationis consistently imported and surfaced in__all__, mirroring changes in the top-level package.
No further issues spotted.Also applies to: 14-15
KaQuMiQ
force-pushed
the
feature/metaprompt
branch
3 times, most recently
from
2eb2299 to
270a78e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/draive/__init__.py (1)
267-267: Exports are correct, but maintenance concerns remain.The new entities are properly added to the
__all__tuple without creating duplicates. However, the previous concerns about maintaining this large hand-maintained tuple still apply.Also applies to: 270-270, 383-383
src/draive/helpers/instruction_preparation.py (4)
49-49: Replace assert with explicit validation for user data.The assert statement will be stripped when Python runs with optimization flags (-O), potentially allowing invalid data to pass through silently.
- assert instruction_declaration.description is not None # nosec: B101 + if instruction_declaration.description is None: + raise ValueError("Instruction description cannot be None")
69-71: Preserve error context when instruction preparation fails.The current error handling discards the LLM output that could be valuable for debugging preparation failures.
- else: - ctx.log_error("...instruction preparation failed!") - raise ValueError("Failed to prepare instruction", result) + else: + ctx.log_error("...instruction preparation failed!") + raise ValueError( + f"Failed to prepare instruction - raw response: {result.to_str() if hasattr(result, 'to_str') else result}" + )
80-82: Fix XML escaping and string concatenation performance.The current implementation has two issues: potential XML injection and quadratic time complexity from repeated string concatenation.
+from html import escape ... - arguments: str = "" - for argument in instruction.arguments: - arguments += f"<{argument.name}>{json.dumps(argument.specification)}</{argument.name}>" + parts = [ + f"<{arg.name}>{escape(json.dumps(arg.specification))}</{arg.name}>" + for arg in instruction.arguments + ] + arguments = "".join(parts)
87-181: Consider externalizing the long instruction template.The
PREPARE_INSTRUCTIONconstant spans nearly 100 lines, which hampers module readability and increases bytecode size. Consider moving it to an external template file.This would improve maintainability and allow for easier prompt editing without code changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/draive/__init__.py(3 hunks)src/draive/helpers/__init__.py(2 hunks)src/draive/helpers/instruction_preparation.py(1 hunks)src/draive/instructions/__init__.py(2 hunks)src/draive/multimodal/tags.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/draive/helpers/instruction_preparation.py (1)
Learnt from: KaQuMiQ
PR: miquido/draive#327
File: src/draive/helpers/instruction_preparation.py:28-34
Timestamp: 2025-05-28T17:41:57.437Z
Learning: The draive project uses and requires Python 3.12+, so PEP-695 generic syntax with square brackets (e.g., `def func[T: Type]()`) is valid and should be used instead of the older TypeVar approach.
🧬 Code Graph Analysis (2)
src/draive/helpers/__init__.py (1)
src/draive/helpers/instruction_preparation.py (2)
InstructionPreparationAmbiguity(18-25)prepare_instruction(28-71)
src/draive/instructions/__init__.py (1)
src/draive/instructions/types.py (1)
InstructionDeclaration(42-46)
🔇 Additional comments (7)
src/draive/helpers/__init__.py (2)
1-4: LGTM! Clean import of new instruction preparation functionality.The imports correctly expose the new instruction preparation functionality from the
instruction_preparationmodule.
16-16: LGTM! Proper export additions.The new entities are correctly added to the
__all__tuple in alphabetical order, maintaining consistency with the existing export pattern.Also applies to: 20-20
src/draive/instructions/__init__.py (2)
5-5: LGTM! Proper import of InstructionDeclaration.The import correctly exposes
InstructionDeclarationfrom the types module, making it accessible for instruction preparation workflows.
14-14: LGTM! Consistent export addition.
InstructionDeclarationis properly added to the__all__tuple in alphabetical order, maintaining the existing pattern.src/draive/__init__.py (1)
104-104: LGTM! Correct umbrella imports for instruction preparation.The new instruction preparation functionality is properly imported and will be available at the top-level
draivenamespace.Also applies to: 108-108, 112-112
src/draive/multimodal/tags.py (2)
422-422: LGTM! Proper extension of tag name parsing.Adding underscore support to tag names is necessary for the new instruction preparation functionality that uses tags like
<RESULT_INSTRUCTION>and<QUESTIONS>.
637-637: LGTM! Consistent parsing logic update.The underscore support is correctly applied to both tag opening and closing parsing functions, maintaining consistency in tag name validation.
KaQuMiQ
force-pushed
the
feature/metaprompt
branch
from
270a78e to
99c8990
Compare
KaQuMiQ
force-pushed
the
feature/metaprompt
branch
from
99c8990 to
5f7f6d8
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/draive/__init__.py (1)
267-267: Exports added correctly without duplication.The new entities are properly added to the
__all__tuple in appropriate alphabetical positions. I verified no duplicates exist.Also applies to: 270-270, 383-383
src/draive/helpers/instruction_preparation.py (4)
49-49:⚠️ Potential issueReplace
assertwith explicit runtime validation
assertstatements are stripped when Python is executed with the-Oflag, so this check would silently disappear in production. Use an explicit conditional and raise a proper exception instead.- assert instruction_declaration.description is not None # nosec: B101 + if instruction_declaration.description is None: # nosec: B101 + raise ValueError("Instruction description cannot be None")
65-67:⚠️ Potential issue
InstructionPreparationAmbiguityexpects a sequence, not a string
parsed.content.to_str()returns a single string, but the exception’s constructor requiresSequence[str]. Convert the raw text into a list (e.g., split by lines or another delimiter) before raising.- raise InstructionPreparationAmbiguity(questions=parsed.content.to_str()) + questions_text = parsed.content.to_str() + questions = [q.strip() for q in questions_text.splitlines() if q.strip()] + raise InstructionPreparationAmbiguity(questions=questions)
80-83: 🛠️ Refactor suggestionQuadratic string concatenation & missing XML escaping
- Re-allocating
argumentson every loop iteration is O(n²).json.dumpsmay emit"<", ">", "&", '"'which break XML unless escaped.- arguments: str = "" - for argument in instruction.arguments: - arguments += f"<{argument.name}>{json.dumps(argument.specification)}</{argument.name}>" + parts = [ + f"<{argument.name}>{escape(json.dumps(argument.specification))}</{argument.name}>" + for argument in instruction.arguments + ] + arguments = "".join(parts)Additional change required outside this hunk:
from html import escape # add near the top with other imports
87-183: 🧹 Nitpick (assertive)Consider externalising the 100-line
PREPARE_INSTRUCTIONpromptKeeping such a large multi-line constant inline harms readability, inflates byte-code size and makes prompt edits error-prone. Loading it from a
.txtor template file (e.g., viaimportlib.resources) would keep the module lean and the prompt easier to maintain.
🛑 Comments failed to post (1)
src/draive/helpers/instruction_preparation.py (1)
50-55:
⚠️ Potential issueUnescaped braces in
guidelinescan breakstr.formatIf the caller passes a guideline string containing single
{}braces (e.g., “Use{key}placeholders”),PREPARE_INSTRUCTION.format()will raiseKeyError.
Escape braces or useformat_mapwith a safe placeholder.- instruction=PREPARE_INSTRUCTION.format( - guidelines=f"\n<GUIDELINES>{guidelines}</GUIDELINES>\n" if guidelines else "", - ), + instruction=PREPARE_INSTRUCTION.format( + guidelines=( + f"\n<GUIDELINES>{guidelines.replace('{', '{{').replace('}', '}}')}</GUIDELINES>\n" + if guidelines else "" + ), + ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.result: MultimodalContent = await Stage.completion( f"<USER_TASK>{instruction_declaration.description}" f"{_format_variables(instruction_declaration)}</USER_TASK>", instruction=PREPARE_INSTRUCTION.format( guidelines=( f"\n<GUIDELINES>" f"{guidelines.replace('{', '{{').replace('}', '}}')}" f"</GUIDELINES>\n" if guidelines else "" ), ), ).execute()🤖 Prompt for AI Agents
In src/draive/helpers/instruction_preparation.py around lines 50 to 55, the guidelines string passed to PREPARE_INSTRUCTION.format() may contain unescaped braces, causing KeyError during string formatting. To fix this, escape any braces in the guidelines string before formatting or replace the use of .format() with .format_map() using a safe dictionary that prevents KeyError from unescaped braces.
1 participant
No description provided.