Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis PR migrates the multimodal model from legacy Media*/Meta* types to a resource- and artifact-centric model across the codebase. It removes media- and tag-element modules, adds ResourceContent/ResourceReference, ArtifactContent, MultimodalTag, and MultimodalContentPart, and updates many providers, generation, evaluation, resource backends/templates/state, MCP, tests, and docs. Several public type signatures and package exports were changed and MIME/encoding semantics tightened; docs were heavily pruned and dependency pins updated. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 74
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (59)
src/draive/parameters/model.py (2)
277-283: Schema now requires additionalProperties: False — add it hereParameterObjectSpecification was tightened to require additionalProperties: False. DataModelMeta-generated specs omit it, risking inconsistency/validation failure downstream.
Apply:
if parameters_specification: data_type.__PARAMETERS_SPECIFICATION__ = { # pyright: ignore[reportAttributeAccessIssue] "type": "object", "properties": parameters_specification, "required": parameters_specification_required, + "additionalProperties": False, }
955-966: Bug: NameError in dataclass branch of _data_strInside the dataclass case, code uses parametrized_data which is undefined in this scope; should use the dataclass instance.
Fix:
case dataclass if is_dataclass(dataclass): string: str = "" for field in dataclass_fields(dataclass): element: str = _data_str( - getattr(parametrized_data, field.name), + getattr(dataclass, field.name), aliased=aliased, converter=None, ).replace("\n", "\n ")src/draive/vllm/messages.py (1)
525-589: Fix: incorrect handling of ResourceReference/ResourceContent for imagesCurrent logic raises on image MIME types and emits image_url for non-images, which inverts expected behavior and will break vision inputs. Emit image_url only for images; degrade others to text.
Apply:
def content_parts( # noqa: C901 @@ - case ResourceReference() as reference: - if text_only: - continue # skip with text only - - if reference.mime_type is not None and reference.mime_type.startswith("image"): - raise ValueError( - f"Unsupported message content mime type: {reference.mime_type}" - ) - - yield { - "type": "image_url", - "image_url": { - "url": reference.uri, - }, - "detail": vision_details, - } + case ResourceReference() as reference: + if text_only: + continue + if reference.mime_type is None or reference.mime_type.startswith("image"): + yield { + "type": "image_url", + "image_url": {"url": reference.uri}, + "detail": vision_details, + } + else: + yield { + "type": "text", + "text": reference.uri, + } @@ - case ResourceContent() as data: - if text_only: - continue # skip with text only - - if data.mime_type is not None and data.mime_type.startswith("image"): - raise ValueError(f"Unsupported message content mime type: {data.mime_type}") - - yield { - "type": "image_url", - "image_url": { - "url": data.to_data_uri(), - }, - "detail": vision_details, - } + case ResourceContent() as data: + if text_only: + continue + if data.mime_type is not None and data.mime_type.startswith("image"): + yield { + "type": "image_url", + "image_url": {"url": data.to_data_uri()}, + "detail": vision_details, + } + else: + yield { + "type": "text", + "text": data.to_str(), # redacted by default + }src/draive/evaluators/fluency.py (1)
42-48: Add NumPy-style docstring to public evaluatorPublic APIs should include Parameters/Returns/Raises per guidelines.
src/draive/evaluators/conciseness.py (1)
49-55: Document the public evaluatorPlease add a NumPy-style docstring covering parameters (evaluated, reference, guidelines), returns (EvaluationScore), and raises.
src/draive/evaluators/readability.py (1)
72-76: Normalize RESULT value before castingTrim/lower to reduce model-output variance and ensure it matches
EvaluationScoreValue.- return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), + return EvaluationScore.of( + cast(EvaluationScoreValue, result.content.to_str().strip().lower()), meta={"comment": completion.to_str()}, )src/draive/evaluators/helpfulness.py (1)
111-115: Normalize RESULT value before castingTrim/lower to stabilize categorical parsing.
- return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), + return EvaluationScore.of( + cast(EvaluationScoreValue, result.content.to_str().strip().lower()), meta={"comment": completion.to_str()}, )src/draive/evaluators/groundedness.py (1)
82-86: Normalize RESULT value before castingSame parsing robustness as other evaluators.
- return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), + return EvaluationScore.of( + cast(EvaluationScoreValue, result.content.to_str().strip().lower()), meta={"comment": completion.to_str()}, )src/draive/evaluators/coverage.py (1)
79-83: Normalize RESULT value before castingKeep categorical output resilient to capitalization/spacing.
- return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), + return EvaluationScore.of( + cast(EvaluationScoreValue, result.content.to_str().strip().lower()), meta={"comment": completion.to_str()}, )src/draive/stages/stage.py (1)
2072-2077: Fail fast on missing SELECTION tag and simplify logicRaise a clearer error when the tag is absent instead of using a sentinel.
- selection_tag = result.content.tag("SELECTION") - selection: str = ( - selection_tag.content.to_str().strip().lower() - if selection_tag is not None - else "__INVALID_SELECTION__" - ) + selection_tag = result.content.tag("SELECTION") + if selection_tag is None: + raise ValueError("Model routing failed: missing <SELECTION> tag in model output") + selection: str = selection_tag.content.to_str().strip().lower()tests/test_resource_uri_template.py (1)
80-91: Reduce duplication via parametrizationSeveral resolve-from-URI tests assert the same
meta.name. Parametrize to cut repetition.-@mark.asyncio -async def test_resolve_from_uri_with_parameters(): +@mark.asyncio +@mark.parametrize( + "template_uri,decorator_name", + [ + ("https://api.example.com/users/{user_id}", "get_user"), + ], +) +async def test_resolve_from_uri_with_parameters(template_uri: str, decorator_name: str): @@ - @resource(uri_template="https://api.example.com/users/{user_id}") - async def get_user(user_id: str): + @resource(uri_template=template_uri) + async def get_user(user_id: str): return ResourceContent.of(f"User {user_id} data") @@ - assert resource_result.meta.name == "get_user" + assert resource_result.meta.name == decorator_namesrc/draive/evaluators/tone_style.py (1)
113-120: Validate RESULT value and avoid blind cast; consider typed exceptionEnsure the extracted rating is one of the allowed values before casting; prefer a custom exception (e.g., EvaluationResultParseError) over ValueError for downstream handling.
Apply within this block:
- if result := completion.tag("RESULT"): - return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), - meta={"comment": completion.to_str()}, - ) - else: - raise ValueError(f"Invalid evaluator result:\n{completion}") + if result := completion.tag("RESULT"): + rating: str = result.content.to_str().strip().lower() + allowed: set[str] = {"poor", "fair", "good", "excellent", "perfect", "none"} + if rating not in allowed: + # TODO: replace ValueError with a typed EvaluationResultParseError + raise ValueError(f"Invalid RESULT rating: {rating!r}") + return EvaluationScore.of( + cast(EvaluationScoreValue, rating), + meta={"comment": completion.to_str()}, + ) + else: + # TODO: replace ValueError with a typed EvaluationResultParseError + raise ValueError(f"Invalid evaluator result:\n{completion}")src/draive/evaluators/coherence.py (1)
81-88: Validate rating and raise typed parse errorMirror the validation pattern to enforce allowed values; prefer a custom parse exception.
- if result := completion.tag("RESULT"): - return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), - meta={"comment": completion.to_str()}, - ) - else: - raise ValueError(f"Invalid evaluator result:\n{completion}") + if result := completion.tag("RESULT"): + rating: str = result.content.to_str().strip().lower() + allowed: set[str] = {"poor", "fair", "good", "excellent", "perfect", "none"} + if rating not in allowed: + raise ValueError(f"Invalid RESULT rating: {rating!r}") + return EvaluationScore.of( + cast(EvaluationScoreValue, rating), + meta={"comment": completion.to_str()}, + ) + else: + raise ValueError(f"Invalid evaluator result:\n{completion}")src/draive/evaluators/factual_accuracy.py (1)
101-108: Add rating validation; consider custom exceptionAvoid trusting model output blindly; validate against the allowed set and use a typed error.
- if result := completion.tag("RESULT"): - return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), - meta={"comment": completion.to_str()}, - ) - else: - raise ValueError(f"Invalid evaluator result:\n{completion}") + if result := completion.tag("RESULT"): + rating: str = result.content.to_str().strip().lower() + allowed: set[str] = {"poor", "fair", "good", "excellent", "perfect", "none"} + if rating not in allowed: + raise ValueError(f"Invalid RESULT rating: {rating!r}") + return EvaluationScore.of( + cast(EvaluationScoreValue, rating), + meta={"comment": completion.to_str()}, + ) + else: + raise ValueError(f"Invalid evaluator result:\n{completion}")src/draive/generation/image/default.py (1)
18-24: Add a NumPy-style docstring for the public APIPublic functions must document Parameters/Returns/Raises.
async def generate_image( *, instructions: ResolveableInstructions, input: MultimodalContent, # noqa: A002 **extra: Any, ) -> ResourceContent | ResourceReference: + """ + Generate a single image using the configured model. + + Parameters + ---------- + instructions : ResolveableInstructions + Resolved or resolvable instructions for the generation request. + input : MultimodalContent + Model input context (prompt + optional resources). + **extra : Any + Provider-specific options forwarded to the model. + + Returns + ------- + ResourceContent | ResourceReference + The first image resource produced by the model. + + Raises + ------ + ValueError + If the model output does not contain an image resource. + """src/draive/generation/audio/default.py (1)
18-24: Add a NumPy-style docstring for the public APIDocument the function per guidelines.
async def generate_audio( *, instructions: ResolveableInstructions, input: MultimodalContent, # noqa: A002 **extra: Any, ) -> ResourceContent | ResourceReference: + """ + Generate a single audio resource using the configured model. + + Parameters + ---------- + instructions : ResolveableInstructions + Resolved or resolvable instructions for the generation request. + input : MultimodalContent + Model input context (prompt + optional resources). + **extra : Any + Provider-specific options forwarded to the model. + + Returns + ------- + ResourceContent | ResourceReference + The first audio resource produced by the model. + + Raises + ------ + ValueError + If the model output does not contain an audio resource. + """src/draive/models/tools/function.py (1)
327-364: Extract allowed multimodal element types into a shared constant and reuse it
- Add near the imports:
_ALLOWED_MULTIMODAL_ELEMENTS = (MultimodalContent, MultimodalTag, TextContent, ResourceReference, ResourceContent, ArtifactContent, str)- Replace the two isinstance checks in src/draive/models/tools/function.py (lines 327–364) with
isinstance(..., _ALLOWED_MULTIMODAL_ELEMENTS).- Ripgrep output shows the same union-style
isinstance(..., A|B)pattern in several files (e.g. src/draive/models/generative.py, src/draive/multimodal/content.py, src/draive/ollama/chat.py, src/draive/helpers/volatile_vector_index.py); consider standardizing those checks as well.src/draive/evaluators/expectations.py (2)
76-84: Validate RESULT value before casting.Guard against unexpected tag contents and normalize casing; raise early on invalid values.
Apply this diff:
- if result := completion.tag("RESULT"): - return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), - meta={"comment": completion.to_str()}, - ) + if result := completion.tag("RESULT"): + allowed = {"poor", "fair", "good", "excellent", "perfect", "none"} + value = result.content.to_str().strip().lower() + if value not in allowed: + raise ValueError( + f"Invalid RESULT rating: {value!r}. Expected one of {sorted(allowed)}.\n{completion}" + ) + return EvaluationScore.of( + cast(EvaluationScoreValue, value), + meta={"comment": completion.to_str()}, + )
42-48: Add NumPy-style docstring to the public evaluator.Public APIs in src/** must have NumPy docstrings (Parameters/Returns/Raises).
src/draive/helpers/instruction_preparation.py (2)
15-23: Document the custom exception.Add a brief NumPy-style docstring and
questionsattribute description.Apply this diff:
class InstructionPreparationAmbiguity(Exception): + """ + Raised when instruction preparation requires user clarification. + + Parameters + ---------- + *args : object + Exception args. + questions : str + Clarification questions to present to the user. + """
25-31: Add NumPy-style docstring forprepare_instructions.Public API should declare Parameters/Returns/Raises.
Apply this diff:
async def prepare_instructions( instruction: InstructionsDeclaration | str, /, *, guidelines: str | None = None, ) -> ModelInstructions: + """ + Prepare system instructions from a task description or declaration. + + Parameters + ---------- + instruction : InstructionsDeclaration | str + Task declaration or plain task description. + guidelines : str | None, optional + Additional preparation guidelines. + + Returns + ------- + ModelInstructions + Prepared instruction text for the model. + + Raises + ------ + InstructionPreparationAmbiguity + When clarification questions are needed instead of instructions. + ValueError + On unexpected formatter or parsing failures. + """src/draive/evaluators/completeness.py (1)
110-117: Validate RESULT value before casting.Mirror validation across evaluators for consistency.
Apply this diff:
- if result := completion.tag("RESULT"): - return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), - meta={"comment": completion.to_str()}, - ) + if result := completion.tag("RESULT"): + allowed = {"poor", "fair", "good", "excellent", "perfect", "none"} + value = result.content.to_str().strip().lower() + if value not in allowed: + raise ValueError( + f"Invalid RESULT rating: {value!r}. Expected one of {sorted(allowed)}.\n{completion}" + ) + return EvaluationScore.of( + cast(EvaluationScoreValue, value), + meta={"comment": completion.to_str()}, + )src/draive/evaluators/consistency.py (2)
46-54: Add a NumPy-style docstring to the public evaluator.Public APIs require docstrings per guidelines.
Apply:
@evaluator(name="consistency") async def consistency_evaluator( evaluated: Multimodal, /, *, reference: Multimodal, guidelines: str | None = None, ) -> EvaluationScore: + """ + Evaluate the consistency of evaluated content against a reference. + + Parameters + ---------- + evaluated : Multimodal + Content to be evaluated. + reference : Multimodal + Reference content used for comparison. + guidelines : str | None, optional + Additional evaluator guidelines. + + Returns + ------- + EvaluationScore + Score derived from a `<RESULT>` tag emitted by the model. + + Raises + ------ + ValueError + If the completion does not contain a valid `<RESULT>` tag/value. + """
81-85: Harden RESULT parsing: trim, validate allowed values, and avoid storing artifacts in meta.Prevents drift from extra whitespace/casing and trims meta size.
Apply:
- if result := completion.tag("RESULT"): - return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), - meta={"comment": completion.to_str()}, - ) + if result := completion.tag("RESULT"): + raw = result.content.to_str().strip() + rating = raw.lower() + allowed = {"poor", "fair", "good", "excellent", "perfect", "none"} + if rating not in allowed: + raise ValueError(f"Invalid RESULT value {raw!r} in:\n{completion}") + return EvaluationScore.of( + cast(EvaluationScoreValue, rating), + meta={"comment": completion.without_artifacts().to_str()}, + )src/draive/evaluators/truthfulness.py (2)
47-55: Add a NumPy-style docstring to the public evaluator.Public APIs require docstrings per guidelines.
Apply:
@evaluator(name="truthfulness") async def truthfulness_evaluator( evaluated: Multimodal, /, *, reference: Multimodal, guidelines: str | None = None, ) -> EvaluationScore: + """ + Evaluate the truthfulness (factual accuracy) of evaluated content. + + Parameters + ---------- + evaluated : Multimodal + Content to be evaluated. + reference : Multimodal + Reference content (used for context; score targets factual accuracy). + guidelines : str | None, optional + Additional evaluator guidelines. + + Returns + ------- + EvaluationScore + Score derived from a `<RESULT>` tag emitted by the model. + + Raises + ------ + ValueError + If the completion does not contain a valid `<RESULT>` tag/value. + """
82-86: Harden RESULT parsing: trim, validate allowed values, and avoid storing artifacts in meta.Mirror consistency evaluator handling to keep behavior uniform.
Apply:
- if result := completion.tag("RESULT"): - return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), - meta={"comment": completion.to_str()}, - ) + if result := completion.tag("RESULT"): + raw = result.content.to_str().strip() + rating = raw.lower() + allowed = {"poor", "fair", "good", "excellent", "perfect", "none"} + if rating not in allowed: + raise ValueError(f"Invalid RESULT value {raw!r} in:\n{completion}") + return EvaluationScore.of( + cast(EvaluationScoreValue, rating), + meta={"comment": completion.without_artifacts().to_str()}, + )src/draive/models/types.py (1)
587-606: Preserve CoT privacy and prior semantics: mark reasoning artifacts hidden.As written, reasoning will surface in textual projections via ArtifactContent.to_str(); previous MetaContent-based flows were hidden. Set hidden=True to avoid leaking chain-of-thought into user-visible text.
Apply:
- def content_with_reasoning(self) -> MultimodalContent: - """\ - Return the multimodal content blocks merged into one and including reasoning as MetaContent - """ + def content_with_reasoning(self) -> MultimodalContent: + """ + Return merged content including reasoning as hidden artifacts. + + Returns + ------- + MultimodalContent + Combined content with reasoning preserved as + ``ArtifactContent(category="reasoning", hidden=True)`` so it does + not affect textual projections while remaining accessible via + ``content.artifacts(...)``. + """ parts: list[MultimodalContentPart] = [] for block in self.blocks: if isinstance(block, MultimodalContent): parts.extend(block.parts) elif isinstance(block, ModelReasoning): parts.append( ArtifactContent.of( block, - category="reasoning", + category="reasoning", + hidden=True, ) ) return MultimodalContent.of(*parts)src/draive/openai/guardrails.py (2)
13-16: Fix public class name typo and export to avoid import breakages.The class and its export are misspelled ("Modereation"). This will break imports across the codebase.
Apply:
-__all__ = ("OpenAIContentModereation",) +__all__ = ("OpenAIContentModeration",) -class OpenAIContentModereation(OpenAIAPI): +class OpenAIContentModeration(OpenAIAPI):
20-27: Add NumPy-style docstring for the public moderation method.Public APIs require NumPy docstrings per guidelines.
async def content_verification( # noqa: C901, PLR0912, PLR0915 self, content: Multimodal, /, *, config: OpenAIModerationConfig | None = None, **extra: Any, ) -> None: + """ + Verify user content using OpenAI's moderation models. + + Parameters + ---------- + content : Multimodal + Content to moderate (text/resources/artifacts). + config : OpenAIModerationConfig, optional + Overrides for model and thresholds. + **extra : Any + Extra request options forwarded to the client. + + Returns + ------- + None + + Raises + ------ + GuardrailsModerationException + If any category breaches configured thresholds. + """src/draive/resources/http.py (1)
60-66: Same rationale for delete: return None instead of NotImplementedError.Keeps behavior consistent with fetch/list.
- # no delete available - raise NotImplementedError("Resource deleting is not available with default implementation") + # Delete not supported by HTTP backend + return Nonesrc/draive/openai/images.py (2)
21-29: Add NumPy-style docstring and note unused input.Public API needs a docstring; also clarify that
inputis currently unused by OpenAI image generation.async def generate_image( self, *, instructions: ResolveableInstructions, input: MultimodalContent, # noqa: A002 config: OpenAIImageGenerationConfig | None = None, **extra: Any, ) -> ResourceContent | ResourceReference: + """ + Generate an image with OpenAI and return it as a resource. + + Parameters + ---------- + instructions : ResolveableInstructions + Prompt or instructions for image generation. + input : MultimodalContent + Currently unused by the OpenAI images.generate endpoint. + config : OpenAIImageGenerationConfig, optional + Provider-specific generation options. + **extra : Any + Extra request options forwarded to the client. + + Returns + ------- + ResourceContent | ResourceReference + Base64 image content or a URL reference depending on `config.result`. + + Raises + ------ + ValueError + If the provider response is missing image content. + """
33-41: Consider passing an explicit timeout if available in config.Maintain consistency with other OpenAI calls using
unwrap_missing(config.timeout).- response: ImagesResponse = await self._client.images.generate( + response: ImagesResponse = await self._client.images.generate( model=generation_config.model, n=1, prompt=await InstructionsRepository.resolve(instructions), quality=generation_config.quality, size=generation_config.size, style=generation_config.style, response_format=generation_config.result, + # timeout=unwrap_missing(generation_config.timeout), # if exposed in config )src/draive/generation/audio/state.py (2)
14-14: Add a NumPy-style class docstring for AudioGenerationPublic classes require NumPy docstrings per guidelines.
Apply:
class AudioGeneration(State): + """ + High-level audio generation facade. + + Notes + ----- + Use ``AudioGeneration.generate(...)`` to produce audio from multimodal input. + """
35-41: Document the public generate() method (NumPy-style)Add Parameters/Returns/Raises.
Apply:
@statemethod async def generate( self, *, instructions: ResolveableInstructions = "", input: Multimodal, # noqa: A002 **extra: Any, ) -> ResourceContent | ResourceReference: + """ + Generate audio from multimodal input. + + Parameters + ---------- + instructions : ResolveableInstructions, optional + Provider-specific instructions or template. Default is an empty string. + input : Multimodal + Input multimodal content to condition generation. + **extra : Any + Provider-specific parameters forwarded to the underlying generator. + + Returns + ------- + ResourceContent | ResourceReference + Generated audio as inline resource or an external reference. + + Raises + ------ + ProviderError + If the underlying provider fails or rejects the request. + """ return await self.generating( instructions=instructions, input=MultimodalContent.of(input), **extra, )src/draive/generation/image/state.py (2)
14-14: Add a NumPy-style class docstring for ImageGenerationPublic classes require NumPy docstrings.
Apply:
class ImageGeneration(State): + """ + High-level image generation facade. + + Notes + ----- + Use ``ImageGeneration.generate(...)`` to produce images from multimodal input. + """
35-41: Document the public generate() method (NumPy-style)Add Parameters/Returns/Raises.
Apply:
@statemethod async def generate( self, *, instructions: ResolveableInstructions = "", input: Multimodal, # noqa: A002 **extra: Any, ) -> ResourceContent | ResourceReference: + """ + Generate an image from multimodal input. + + Parameters + ---------- + instructions : ResolveableInstructions, optional + Provider-specific instructions or template. Default is an empty string. + input : Multimodal + Input multimodal content to condition generation. + **extra : Any + Provider-specific parameters forwarded to the underlying generator. + + Returns + ------- + ResourceContent | ResourceReference + Generated image as inline resource or an external reference. + + Raises + ------ + ProviderError + If the underlying provider fails or rejects the request. + """ return await self.generating( instructions=instructions, input=MultimodalContent.of(input), **extra, )src/draive/helpers/instruction_refinement.py (5)
468-487: Fix typos in prompt textTypos reduce model reliability and polish.
Apply:
-You are an expert prompt engineer providing feedback and recemmondations for instructions improvement. +You are an expert prompt engineer providing feedback and recommendations for instructions improvement. @@ -<name>Examplification</name> +<name>Exemplification</name> @@ -<name>Constraning</name> +<name>Constraining</name>
36-52: Convert docstring to NumPy style for public APIPublic functions must use NumPy docstrings.
Apply:
- """ - Refine instructions using binary tree exploration with performance pruning. - - Each node generates exactly 2 complementary strategies. - Branches are pruned if performance drops below threshold. - - Args: - instructions: The instructions to refine - guidelines: Optional guidelines for refinement - evaluator_suite: Suite to evaluate instructions performance - rounds_limit: Maximum depth of refinement tree - sample_ratio: Fraction of passing cases to include in focused evaluation - candidates_limit: Number of top candidates to fully evaluate - performance_drop_threshold: Prune branches with score drop > this threshold - quality_threshold: Stop if score reaches this threshold - concurrent_nodes: How many nodes explored concurrently - """ + """ + Refine instructions using a tree search with performance-based pruning. + + Parameters + ---------- + instructions : Instructions + The instructions subject to refinement. + instructions_content : ModelInstructions | None, optional + Preloaded content for ``instructions``; if ``None``, it is loaded. + guidelines : str | None, optional + Optional refinement guidelines injected into prompts. + evaluator_suite : PreparedEvaluatorSuite[Parameters] + Suite used to evaluate instruction performance. + evaluator_cases : Sequence[EvaluatorSuiteCase[Parameters]] + All available evaluator cases. + rounds_limit : int + Maximum number of exploration rounds. + sample_ratio : float, optional + Fraction of passing cases sampled for focused evaluation. Default is 0.1. + candidates_limit : int, optional + Number of top candidates for full evaluation. Default is 3. + performance_drop_threshold : float, optional + Prune branches when focused score ratio drops below this value. Default is 0.5. + quality_threshold : float, optional + Early-stop threshold on score. Default is 0.99. + concurrent_nodes : int, optional + Number of nodes explored concurrently. Default is 1. + + Returns + ------- + ModelInstructions + The best-performing refined instructions. + + Raises + ------ + AssertionError + If input parameters fail validation. + """
563-565: Avoid logging full generated instructionsLog length or a short preview to reduce noise and accidental leakage.
Apply:
- ctx.log_info(f"Updated instruction for {strategy_name}: {updated_instruction}") + ctx.log_info(f"Updated instruction for {strategy_name}: length={len(updated_instruction)} chars")
155-162: Remove unused rounds_remaining from refinement stateThe field is never referenced; drop it and its initialization.
Apply:
class _RefinementState(State): root: _RefinementTreeNode nodes: Mapping[UUID, _RefinementTreeNode] sample_ratio: float performance_drop_threshold: float guidelines: str | None = None - rounds_remaining: int @@ return state.updated( _RefinementState( root=root_node, nodes={root_node.identifier: root_node}, sample_ratio=sample_ratio, performance_drop_threshold=performance_drop_threshold, guidelines=guidelines, - rounds_remaining=rounds_limit, ), # set the result to be initial instruction content result=MultimodalContent.of(content), )Also applies to: 261-269
163-165: Rename leafs() -> leaves()Minor naming nit for clarity; update usage.
Apply:
- def leafs(self) -> Sequence[_RefinementTreeNode]: + def leaves(self) -> Sequence[_RefinementTreeNode]: return [node for node in self.nodes.values() if node.is_leaf and not node.pruned] @@ - node_updates: Sequence[Mapping[UUID, _RefinementTreeNode]] = await execute_concurrently( + node_updates: Sequence[Mapping[UUID, _RefinementTreeNode]] = await execute_concurrently( explore, - refinement_state.leafs(), + refinement_state.leaves(), concurrent_tasks=concurrent_nodes, )Also applies to: 307-311
src/draive/helpers/volatile_vector_index.py (2)
73-78: Bug: invalid isinstance() use with a union type will raise TypeError at runtime.
isinstance(value, bytes | bytearray)is illegal. Use a tuple.- elif all(isinstance(value, bytes | bytearray) for value in selected_values): + elif all(isinstance(value, (bytes, bytearray)) for value in selected_values):
171-174: Guard the fallback pattern to ensurequeryis a sequence of numbers.The bare
case vector:captures anything and can mask type errors. Validate numeric sequences and fail fast otherwise.- case vector: - query_vector = vector + case [*nums] if all(isinstance(n, (int, float)) for n in nums): + query_vector = [float(n) for n in nums] + case other: + raise TypeError( + "query must be a sequence[float], str, TextContent, or ResourceContent" + )src/draive/evaluators/similarity.py (1)
84-88: Harden RESULT parsing: trim/validate before casting.
.to_str()may include whitespace/newlines. Normalize and validate against allowed values to avoid invalid casts.- if result := completion.tag("RESULT"): - return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), - meta={"comment": completion.to_str()}, - ) + if result := completion.tag("RESULT"): + raw = result.content.to_str().strip().lower() + allowed: tuple[EvaluationScoreValue, ...] = ("none", "poor", "good", "perfect") + if raw not in allowed: + raise InvalidEvaluatorResultError(f"Unexpected RESULT value: {raw!r}") + return EvaluationScore.of(cast(EvaluationScoreValue, raw), meta={"comment": completion.to_str()})Add once near the imports:
class InvalidEvaluatorResultError(ValueError): """Raised when the evaluator completion lacks a valid <RESULT> tag."""src/draive/mistral/completions.py (2)
439-465: Allow image URLs without mime_type; ensure ResourceContent path remains image-only.Current match excludes ResourceReference without a mime_type (typical for plain image URLs) and silently degrades to text. Loosen the guard for references.
- case ResourceReference() as ref if (ref.mime_type or "").startswith("image"): + case ResourceReference() as ref if (ref.mime_type is None) or ref.mime_type.startswith("image"): yield { "type": "image_url", "image_url": { "url": ref.uri, }, } @@ - case ResourceContent() as data if (data.mime_type or "").startswith("image"): + case ResourceContent() as data if data.mime_type.startswith("image"): yield { "type": "image_url", "image_url": { # ResourceContent.to_data_uri() returns a proper data URI "url": data.to_data_uri(), }, }
482-503: Treat data: URLs explicitly to avoid provider ambiguity (optional).If Mistral rejects data: URLs in image_url, we may need to detect and emit ResourceContent instead. Optional unless tests/providers require it.
if ctype == "image_url": image = cast(Mapping[str, Any] | None, chunk.get("image_url")) or {} url = cast(str | None, image.get("url")) or "" if url: - # We cannot reliably distinguish data URIs here without parsing; - # treat as a reference and mark mime type as generic image. - return ResourceReference.of(url, mime_type="image/*") + if url.startswith("data:"): + # Keep it inline to avoid remote fetch; mime type is carried in the URI + return ResourceContent.of(b"", mime_type="image/*", meta={"uri": url}) + # Otherwise treat as a reference and mark mime type as generic image. + return ResourceReference.of(url, mime_type="image/*")src/draive/openai/realtime.py (2)
569-579: Bug: dict mutated during iteration in _reset_contextYou’re deleting keys from
current_itemswhile iterating its.keys()view, which can raise “dictionary changed size during iteration”.- for item_id in current_items.keys(): + for item_id in list(current_items.keys()): try: await connection.conversation.item.delete(item_id=item_id) - del current_items[item_id] + current_items.pop(item_id, None)
91-95: Nit: unify naming for output audio mime stringYou define both
output_audio_typeandoutput_audio_formatas"audio/{...}". Keep one to avoid confusion.- # prepare output audio format - output_audio_type: str = f"audio/{config.output_audio_format}" + # prepare output audio mime + output_audio_type: str = f"audio/{config.output_audio_format}" ... - output_audio_format: str = f"audio/{config.output_audio_format}" + output_audio_format: str = output_audio_typeAlso applies to: 143-144
src/draive/utils/vector_index.py (1)
33-38: Docstrings missing for public APIPer guidelines, add NumPy-style docstrings for
VectorIndexing.__call__,VectorSearching.__call__, andVectorIndex.search(and similarly forindex/delete) to document accepted unions and behavior.Would you like me to draft concise NumPy-style docstrings for these symbols?
Also applies to: 125-144, 151-156
src/draive/bedrock/converse.py (4)
46-48: Add NumPy-style docstring for public method.Public APIs must have docstrings. Please document
generative_model().def generative_model(self) -> GenerativeModel: - return GenerativeModel(generating=self.completion) + """ + Return a GenerativeModel bound to this Bedrock provider. + + Returns + ------- + GenerativeModel + Model that delegates to ``completion(...)``. + """ + return GenerativeModel(generating=self.completion)
77-89: Document the publiccompletion(...)method.Add a concise NumPy-style docstring covering Parameters/Returns.
def completion( self, *, instructions: ModelInstructions, tools: ModelToolsDeclaration, context: ModelContext, output: ModelOutputSelection, stream: bool = False, config: BedrockChatConfig | None = None, prefill: Multimodal | None = None, **extra: Any, ) -> AsyncGenerator[ModelStreamOutput] | Coroutine[None, None, ModelOutput]: + """ + Generate a completion using Amazon Bedrock Converse. + + Parameters + ---------- + instructions : ModelInstructions + tools : ModelToolsDeclaration + context : ModelContext + output : ModelOutputSelection + stream : bool, default False + config : BedrockChatConfig | None + prefill : Multimodal | None + + Returns + ------- + Coroutine[None, None, ModelOutput] or AsyncGenerator[ModelStreamOutput] + Non-streaming output or a stream of output chunks. + """
188-209: Guard metrics access when usage is absent.Some Bedrock responses omit usage; current code risks KeyError.
- ctx.record( + usage = completion.get("usage", {}) + ctx.record( ObservabilityLevel.INFO, metric="model.input_tokens", - value=completion["usage"]["inputTokens"], + value=usage.get("inputTokens", 0), unit="tokens", kind="counter", attributes={ "model.provider": "bedrock", "model.name": config.model, }, ) - ctx.record( + ctx.record( ObservabilityLevel.INFO, metric="model.output_tokens", - value=completion["usage"]["outputTokens"], + value=usage.get("outputTokens", 0), unit="tokens", kind="counter", attributes={ "model.provider": "bedrock", "model.name": config.model, }, )
216-233: Handle all Bedrock stopReason values — don't assume only 'guardrail_intervened'/'content_filtered'.models.py declares stopReason Literal: "end_turn", "tool_use", "max_tokens", "stop_sequence", "guardrail_intervened", "content_filtered". Update src/draive/bedrock/converse.py (around lines 214–233) to explicitly handle or document how "end_turn", "tool_use", and "stop_sequence" are treated (or add a clear default/fallback) instead of only checking "max_tokens" and the two guardrail values.
src/draive/resources/template.py (3)
413-440: Public API: rename uri_template → template_uri (+compat).The rest of this module uses template_uri. Align the decorator’s parameter name and keep backward compatibility.
Apply:
-def resource[**Args]( - *, - uri_template: str, - mime_type: str | None = None, +def resource[**Args]( + *, + template_uri: str | None = None, + uri_template: str | None = None, # DEPRECATED + mime_type: MimeType | None = None, name: str | None = None, description: str | None = None, availability_check: ResourceAvailabilityCheck | None = None, meta: Meta | MetaValues | None = None, ) -> Callable[ [Callable[Args, Coroutine[None, None, Sequence[ResourceReference] | ResourceContent]]], ResourceTemplate[Args], ]: - def wrap( + def wrap( function: Callable[ Args, Coroutine[None, None, Sequence[ResourceReference] | ResourceContent] ], ) -> ResourceTemplate[Args]: + tpl = template_uri or uri_template + if tpl is None: + raise ValueError("template_uri must be provided") return ResourceTemplate[Args]( - uri_template, + tpl, mime_type=mime_type, name=name or function.__name__, description=description, availability_check=availability_check, function=function, meta=Meta.of(meta), )
369-387: Type validation missing in resolve_from_uri().Extracted URI params are str; they should be validated/coerced via validate_arguments before calling the function.
Apply:
- # If we have parameters, try to call with them - if uri_params: - content = await self.__call__(**uri_params) # pyright: ignore[reportCallIssue] - - else: - content = await self.__call__() # pyright: ignore[reportCallIssue] + # Validate/coerce parameters before calling + values: Mapping[str, Any] = self.validate_arguments(**uri_params) if uri_params else {} + content = await self.__call__(**values) # pyright: ignore[reportCallIssue]
262-276: Path template may include query/fragment; extraction should stop before ?/#.current logic can fold {?…}/{#…} into the path regex and break matching. Trim at the first ? or # after netloc.
Apply:
def _extract_path_template(self) -> str: - if "://" in self.declaration.template_uri: - reminder = self.declaration.template_uri.split("://", 1)[1] - # Find where the netloc ends (first occurrence of {, /, ?, or #) - netloc_end = len(reminder) - for char in ["/", "{", "?", "#"]: - pos = reminder.find(char) - if pos != -1 and pos < netloc_end: - netloc_end = pos - - return reminder[netloc_end:] + if "://" in self.declaration.template_uri: + reminder = self.declaration.template_uri.split("://", 1)[1] + # find end of netloc + netloc_end = len(reminder) + for ch in ("/", "{", "?", "#"): + pos = reminder.find(ch) + if pos != -1 and pos < netloc_end: + netloc_end = pos + rest = reminder[netloc_end:] + # cut off query/fragment from path template + path_end = len(rest) + for ch in ("?", "#"): + pos = rest.find(ch) + if pos != -1 and pos < path_end: + path_end = pos + return rest[:path_end] else: return self.declaration.template_urisrc/draive/mcp/client.py (3)
323-347: Use ResourceContent.of for consistent, urlsafe encoding (text case).Manual base64 here differs from ResourceContent.of behavior (urlsafe). Use the factory.
Apply:
- return ResourceContent( - data=b64encode(text_resource.text.encode()).decode(), - mime_type=text_resource.mimeType or "text/plain", - meta=Meta( - { - "mcp_server": self.identifier, - "tags": self.tags, - } - ), - ) + return ResourceContent.of( + text_resource.text, + mime_type=text_resource.mimeType or "text/plain", + meta=Meta( + { + "mcp_server": self.identifier, + "tags": self.tags, + } + ), + )
337-347: Use ResourceContent.of and decode blob with urlsafe_b64decode (binary case).Ensure consistent encoding semantics and support both base64 variants.
Apply:
- return ResourceContent( - data=blob_resource.blob, - mime_type=blob_resource.mimeType or "application/octet-stream", - meta=Meta( - { - "mcp_server": self.identifier, - "tags": self.tags, - } - ), - ) + return ResourceContent.of( + urlsafe_b64decode(blob_resource.blob), + mime_type=blob_resource.mimeType or "application/octet-stream", + meta=Meta( + { + "mcp_server": self.identifier, + "tags": self.tags, + } + ), + )
430-458: Minor consistency: prefer TextContent.of in conversions.Not critical, but aligns with the rest of the codebase style.
Apply:
- return MultimodalContent.of(TextContent(text=text.text)) + return MultimodalContent.of(TextContent.of(text.text)) ... - return MultimodalContent.of( - TextContent(text=urlsafe_b64decode(blob.blob).decode()) - ) + return MultimodalContent.of( + TextContent.of(urlsafe_b64decode(blob.blob).decode()) + )
| - Build content with `MultimodalContent.of(...)` and prefer composing content blocks explicitly | ||
| - Use ResourceContent/Reference for media and data blobs | ||
| - Wrap custom types and data within ArtifactContent, use hidden when needed | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify naming: ResourceReference
Spell out ResourceReference to match API nouns elsewhere.
-- Use ResourceContent/Reference for media and data blobs
+- Use ResourceContent/ResourceReference for media and data blobs📝 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.
| - Build content with `MultimodalContent.of(...)` and prefer composing content blocks explicitly | |
| - Use ResourceContent/Reference for media and data blobs | |
| - Wrap custom types and data within ArtifactContent, use hidden when needed | |
| - Build content with `MultimodalContent.of(...)` and prefer composing content blocks explicitly | |
| - Use ResourceContent/ResourceReference for media and data blobs | |
| - Wrap custom types and data within ArtifactContent, use hidden when needed |
🤖 Prompt for AI Agents
In AGENTS.md around lines 122 to 125, the doc uses the term
"ResourceContent/Reference" — rename and spell out "ResourceReference"
consistently to match API nouns elsewhere; update the text to explicitly use
"ResourceReference" (singular, capitalized) wherever the combined term appears
and adjust surrounding wording so examples and descriptions reference
ResourceReference as the official noun.
| raw_bytes = file.read() | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer Path.read_bytes() for brevity
Cleaner and safer than manual open/close.
- with open(image_path, "rb") as file:
- raw_bytes = file.read()
+ raw_bytes = image_path.read_bytes()📝 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.
| raw_bytes = file.read() | |
| raw_bytes = image_path.read_bytes() |
🤖 Prompt for AI Agents
In docs/getting-started/first-steps.md around lines 195 to 196 the snippet uses
manual file.open() and read() to obtain raw_bytes; replace that pattern with
Path.read_bytes() on the Path object to make the code shorter and safer (removes
explicit open/close and context management) — update the example to call the
Path instance's read_bytes() method and use its returned bytes where raw_bytes
is needed.
| curl -fLsS https://astral.sh/uv/install.sh | sh; \ | ||
| if [ $$? -ne 0 ]; then \ |
There was a problem hiding this comment.
Installer no longer pins UV_VERSION — breaks reproducibility
You compare against UV_VERSION but install “latest” (no --version), which may overshoot the desired version and diverge across machines/CI.
Pin install/update to UV_VERSION:
- curl -fLsS https://astral.sh/uv/install.sh | sh; \
+ curl -fLsS https://astral.sh/uv/install.sh | sh -s -- --version $(UV_VERSION); \
...
- curl -fLsS https://astral.sh/uv/install.sh | sh; \
+ curl -fLsS https://astral.sh/uv/install.sh | sh -s -- --version $(UV_VERSION); \Also applies to: 37-38
🤖 Prompt for AI Agents
In Makefile around lines 25-26 (and similarly lines 37-38), the installer curl
pipes the install script without passing a version, so it installs the latest UV
release while the script later checks against UV_VERSION; change the install
invocation to pass the UV_VERSION environment/config variable (e.g., add
--version "$$UV_VERSION" or the installer’s equivalent flag) so the
installed/updated UV binary is pinned to UV_VERSION, and ensure error handling
and the subsequent version check still work with the pinned install.
| case ResourceContent() as content_part: | ||
| # Only selected image resources are supported by Bedrock guardrails | ||
| if content_part.mime_type == "image/jpeg": | ||
| moderated_content.append( | ||
| { | ||
| "image": { | ||
| "format": "jpeg", | ||
| "source": {"bytes": media_data.data}, | ||
| "source": {"bytes": urlsafe_b64decode(content_part.data)}, | ||
| } | ||
| } | ||
| ) | ||
|
|
||
| elif media_data.media.startswith("image/png"): | ||
| elif content_part.mime_type == "image/png": | ||
| moderated_content.append( | ||
| { | ||
| "image": { | ||
| "format": "png", | ||
| "source": {"bytes": media_data.data}, | ||
| "source": {"bytes": urlsafe_b64decode(content_part.data)}, | ||
| } | ||
| } | ||
| ) | ||
|
|
||
| elif content_part.mime_type == "image/gif": | ||
| moderated_content.append( | ||
| { | ||
| "image": { | ||
| "format": "gif", | ||
| "source": {"bytes": urlsafe_b64decode(content_part.data)}, | ||
| } | ||
| } | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Deduplicate image handling and add robust base64 decoding.
Reduce repetition and avoid crashes on invalid base64 by falling back to JSON text.
Apply this diff:
- case ResourceContent() as content_part:
- # Only selected image resources are supported by Bedrock guardrails
- if content_part.mime_type == "image/jpeg":
- moderated_content.append(
- {
- "image": {
- "format": "jpeg",
- "source": {"bytes": urlsafe_b64decode(content_part.data)},
- }
- }
- )
-
- elif content_part.mime_type == "image/png":
- moderated_content.append(
- {
- "image": {
- "format": "png",
- "source": {"bytes": urlsafe_b64decode(content_part.data)},
- }
- }
- )
-
- elif content_part.mime_type == "image/gif":
- moderated_content.append(
- {
- "image": {
- "format": "gif",
- "source": {"bytes": urlsafe_b64decode(content_part.data)},
- }
- }
- )
-
- else:
+ case ResourceContent() as content_part:
+ # Only selected image resources are supported by Bedrock guardrails
+ _format_map = {"image/jpeg": "jpeg", "image/png": "png", "image/gif": "gif"}
+ if content_part.mime_type in _format_map:
+ try:
+ decoded = urlsafe_b64decode(content_part.data)
+ except Exception as exc:
+ ctx.log_warning(
+ f"Invalid base64 for {content_part.mime_type}: {exc}; "
+ "verifying as json text..."
+ )
+ moderated_content.append(
+ {"text": {"text": content_part.to_json(), "qualifiers": [qualifier]}}
+ )
+ else:
+ moderated_content.append(
+ {
+ "image": {
+ "format": _format_map[content_part.mime_type],
+ "source": {"bytes": decoded},
+ }
+ }
+ )
+ else:
ctx.log_warning(
"Attempting to use guardrails on unsupported media "
f"({content_part.mime_type}), verifying as json text..."
)
moderated_content.append(
{
"text": {
"text": content_part.to_json(),
"qualifiers": [qualifier],
}
}
)📝 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.
| case ResourceContent() as content_part: | |
| # Only selected image resources are supported by Bedrock guardrails | |
| if content_part.mime_type == "image/jpeg": | |
| moderated_content.append( | |
| { | |
| "image": { | |
| "format": "jpeg", | |
| "source": {"bytes": media_data.data}, | |
| "source": {"bytes": urlsafe_b64decode(content_part.data)}, | |
| } | |
| } | |
| ) | |
| elif media_data.media.startswith("image/png"): | |
| elif content_part.mime_type == "image/png": | |
| moderated_content.append( | |
| { | |
| "image": { | |
| "format": "png", | |
| "source": {"bytes": media_data.data}, | |
| "source": {"bytes": urlsafe_b64decode(content_part.data)}, | |
| } | |
| } | |
| ) | |
| elif content_part.mime_type == "image/gif": | |
| moderated_content.append( | |
| { | |
| "image": { | |
| "format": "gif", | |
| "source": {"bytes": urlsafe_b64decode(content_part.data)}, | |
| } | |
| } | |
| ) | |
| case ResourceContent() as content_part: | |
| # Only selected image resources are supported by Bedrock guardrails | |
| _format_map = {"image/jpeg": "jpeg", "image/png": "png", "image/gif": "gif"} | |
| if content_part.mime_type in _format_map: | |
| try: | |
| decoded = urlsafe_b64decode(content_part.data) | |
| except Exception as exc: | |
| ctx.log_warning( | |
| f"Invalid base64 for {content_part.mime_type}: {exc}; " | |
| "verifying as json text..." | |
| ) | |
| moderated_content.append( | |
| {"text": {"text": content_part.to_json(), "qualifiers": [qualifier]}} | |
| ) | |
| else: | |
| moderated_content.append( | |
| { | |
| "image": { | |
| "format": _format_map[content_part.mime_type], | |
| "source": {"bytes": decoded}, | |
| } | |
| } | |
| ) | |
| else: | |
| ctx.log_warning( | |
| "Attempting to use guardrails on unsupported media " | |
| f"({content_part.mime_type}), verifying as json text..." | |
| ) | |
| moderated_content.append( | |
| { | |
| "text": { | |
| "text": content_part.to_json(), | |
| "qualifiers": [qualifier], | |
| } | |
| } | |
| ) |
🤖 Prompt for AI Agents
In src/draive/bedrock/guardrails.py around lines 145 to 175, the image handling
logic repeats identical dict construction for each mime type and will crash on
invalid base64 data; refactor to map supported mime types to formats, check if
content_part.mime_type is in that map and append a single constructed image dict
using the mapped format, and replace direct urlsafe_b64decode with a safe decode
wrapper that catches binascii.Error/TypeError and instead uses {"text":
content_part.data} (or raw JSON text) as the source fallback so invalid base64
does not raise.
| elif isinstance(content, str): | ||
| resource_content = ResourceContent.of( | ||
| content, | ||
| mime_type=mime_type if mime_type is not None else "application/octet-stream", | ||
| ) | ||
|
|
||
| else: | ||
| assert isinstance(content, Collection | ResourceContent) # nosec: B101 | ||
| resource_content = content | ||
|
|
There was a problem hiding this comment.
Default MIME type for str content should be text/plain
Resource.of(str, mime_type=None) currently sets application/octet-stream; this conflicts with ResourceContent.of and will mislabel text as binary.
Apply:
- elif isinstance(content, str):
- resource_content = ResourceContent.of(
- content,
- mime_type=mime_type if mime_type is not None else "application/octet-stream",
- )
+ elif isinstance(content, str):
+ resource_content = ResourceContent.of(
+ content,
+ mime_type=mime_type if mime_type is not None else "text/plain",
+ )📝 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.
| elif isinstance(content, str): | |
| resource_content = ResourceContent.of( | |
| content, | |
| mime_type=mime_type if mime_type is not None else "application/octet-stream", | |
| ) | |
| else: | |
| assert isinstance(content, Collection | ResourceContent) # nosec: B101 | |
| resource_content = content | |
| elif isinstance(content, str): | |
| resource_content = ResourceContent.of( | |
| content, | |
| mime_type=mime_type if mime_type is not None else "text/plain", | |
| ) | |
| else: | |
| assert isinstance(content, Collection | ResourceContent) # nosec: B101 | |
| resource_content = content |
🤖 Prompt for AI Agents
In src/draive/resources/types.py around lines 285 to 294, the branch that wraps
a Python str into ResourceContent currently defaults mime_type to
"application/octet-stream"; change it to default to "text/plain" when mime_type
is None so text content is labeled correctly and stays consistent with
ResourceContent.of; update the conditional to pass mime_type=(mime_type if
mime_type is not None else "text/plain") to ResourceContent.of (only for the str
branch) and leave other branches unchanged.
| else: | ||
| assert isinstance(content, Collection | ResourceContent) # nosec: B101 | ||
| resource_content = content | ||
|
|
There was a problem hiding this comment.
Bug: isinstance with union type
isinstance(..., Collection | ResourceContent) raises TypeError. Use a tuple.
Apply:
- assert isinstance(content, Collection | ResourceContent) # nosec: B101
+ assert isinstance(content, (Collection, ResourceContent)) # nosec: B101📝 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.
| else: | |
| assert isinstance(content, Collection | ResourceContent) # nosec: B101 | |
| resource_content = content | |
| else: | |
| assert isinstance(content, (Collection, ResourceContent)) # nosec: B101 | |
| resource_content = content |
🤖 Prompt for AI Agents
In src/draive/resources/types.py around lines 291 to 294, the isinstance call
uses a union type (Collection | ResourceContent) which raises a TypeError;
replace it with a tuple of types so isinstance can accept it (e.g.
isinstance(content, (Collection, ResourceContent))). Update the assertion line
accordingly and run tests to ensure no other places use the union form with
isinstance.
| content=MultimodalContent.of( | ||
| ResourceReference.of(uri="https://miquido.com/image", mime_type="image/png") | ||
| ), |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
URL resource test cases look correct; consider adding name/description coverage
Construction via ResourceReference.of(..., mime_type="image/png") and expected JSON (including meta.name/meta.description) match ResourceReference semantics. Add one test with non-None name/description to lock the shape.
Also applies to: 300-304
🤖 Prompt for AI Agents
In tests/test_model.py around lines 287-289 (and similarly for lines 300-304),
the ResourceReference test cases only cover uri and mime_type; add at least one
test case that constructs ResourceReference.of(...) with non-None name and
description (e.g., name="foo", description="bar") used inside
MultimodalContent.of(...), and update/extend the expected JSON assertions to
include meta.name and meta.description to lock the serialized shape; ensure the
test asserts both presence and correct values for meta.name and
meta.description.
| "type": "integer", | ||
| } | ||
| }, | ||
| "additionalProperties": False, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Tighten object schema — consider adding a negative test case
Good addition. Please add a companion test asserting that inputs with unexpected keys are rejected for this schema to ensure enforcement is covered by tests.
🤖 Prompt for AI Agents
In tests/test_validation.py around line 81, the schema now sets
"additionalProperties": False but lacks a negative test; add a new test that
constructs a valid base object for this schema, then inserts an unexpected extra
key (e.g., {"unexpected": "value"}) and asserts validation fails (raise/returns
error) — use the same validation helper or jsonschema.validate and assert it
raises jsonschema.exceptions.ValidationError (or the project's validation error)
so the schema rejection of additional properties is covered.
| "Resource", | ||
| "Resource", | ||
| "ResourceAvailabilityCheck", | ||
| "ResourceContent", | ||
| "ResourceContent", | ||
| "ResourceDeclaration", | ||
| "ResourceException", | ||
| "ResourceCorrupted", | ||
| "ResourceDeleting", | ||
| "ResourceFetching", | ||
| "ResourceListFetching", | ||
| "ResourceMissing", | ||
| "ResourceMissing", | ||
| "ResourceReference", | ||
| "ResourceReference", | ||
| "ResourceReferenceTemplate", | ||
| "ResourceTemplate", | ||
| "ResourceTemplateDeclaration", | ||
| "Resources", | ||
| "ResourceTemplate", | ||
| "ResourceUploading", | ||
| "ResourcesRepository", | ||
| "ResourcesRepository", | ||
| "ScopeContext", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Deduplicate all entries.
__all__ currently has duplicates for several symbols (Resource, ResourceContent, ResourceMissing, ResourceReference, ResourceTemplate, ResourcesRepository, resource). This can confuse tooling and reviewers.
Apply:
"Resource",
- "Resource",
"ResourceAvailabilityCheck",
- "ResourceContent",
"ResourceContent",
"ResourceCorrupted",
"ResourceDeleting",
"ResourceFetching",
"ResourceListFetching",
- "ResourceMissing",
"ResourceMissing",
- "ResourceReference",
"ResourceReference",
"ResourceReferenceTemplate",
"ResourceTemplate",
- "ResourceTemplate",
"ResourceUploading",
- "ResourcesRepository",
"ResourcesRepository",
...
"resource",
- "resource",Run to confirm no duplicates remain:
Also applies to: 387-389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import importlib, inspect
m = importlib.import_module("src.draive.__init__".replace("/", ".").removesuffix(".__init__"))
names = list(getattr(m, "__all__", ()))
dups = {n for n in names if names.count(n) > 1}
print("DUPLICATES:", sorted(dups))
PYLength of output: 856
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/draive/__init__.py"
if [ ! -f "$file" ]; then
echo "MISSING_FILE:$file"
exit 0
fi
python - <<'PY'
import ast, sys
path = "src/draive/__init__.py"
src = open(path, "r", encoding="utf-8").read()
tree = ast.parse(src, filename=path)
names_list = None
for node in tree.body:
if isinstance(node, ast.Assign):
for target in node.targets:
if isinstance(target, ast.Name) and target.id == "__all__":
val = node.value
names = []
if isinstance(val, (ast.List, ast.Tuple)):
for elt in val.elts:
try:
names.append(ast.literal_eval(elt))
except Exception:
names.append("<UNPARSEABLE>")
else:
try:
names = list(ast.literal_eval(val))
except Exception as e:
print("UNABLE_PARSE_ALL:", e)
names = []
names_list = names
break
if names_list is None:
print("NO___ALL__FOUND")
sys.exit(0)
dups = sorted({n for n in names_list if names_list.count(n) > 1})
print("TOTAL_COUNT:", len(names_list))
print("UNIQUE_COUNT:", len(set(names_list)))
print("DUPLICATES:", dups)
for i, n in enumerate(names_list, start=1):
print(f"{i:04d}: {n}")
PYLength of output: 4327
Deduplicate all in src/draive/init.py
all contains duplicate entries: Resource, ResourceContent, ResourceMissing, ResourceReference, ResourceTemplate, ResourcesRepository, resource — remove the duplicate occurrences so each exported name appears exactly once. File: src/draive/init.py (around lines 322–341 and 387–389).
🤖 Prompt for AI Agents
In src/draive/__init__.py around lines 322–341 and 387–389, the __all__ list
contains duplicate exported names (Resource, ResourceContent, ResourceMissing,
ResourceReference, ResourceTemplate, ResourcesRepository, resource); remove the
repeated entries so that each name appears exactly once in the __all__ sequence;
edit both places where the list is defined/extended to deduplicate entries
(keeping one occurrence of each export and preserving intended order) so the
module exports are unique.
| image_block: ImageBlockParam = { | ||
| "type": "image", | ||
| "source": { | ||
| "type": "base64", | ||
| "media_type": cast(Any, part.media), | ||
| "data": b64encode(part.data).decode(), | ||
| "media_type": cast(Any, part.mime_type), | ||
| "data": part.data, # ResourceContent.data is already base64-encoded | ||
| }, | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Normalize urlsafe base64 to standard base64 for Anthropic payloads.
ResourceContent.data may be urlsafe-encoded (see ResourceContent.of). Anthropic’s “base64” source expects standard base64. Normalize to avoid sporadic provider errors.
image_block: ImageBlockParam = {
"type": "image",
"source": {
"type": "base64",
- "media_type": cast(Any, part.mime_type),
- "data": part.data, # ResourceContent.data is already base64-encoded
+ "media_type": cast(Any, part.mime_type),
+ # Normalize to standard base64 (padding + alphabet) to satisfy API expectations
+ "data": _normalize_base64(part.data),
},
}Add this helper (outside the shown hunk):
def _normalize_base64(data: str) -> str:
std = data.replace("-", "+").replace("_", "/")
return std + "=" * (-len(std) % 4)To double-check provider requirements:
🌐 Web query:
Anthropic Messages API image block "base64" source: does the API accept URL-safe base64 or require standard base64? Provide official docs link and example.
💡 Result:
Short answer: Anthropic’s Messages API expects standard Base64 (not URL-safe). The docs show examples using the standard base64 encoder (e.g., output beginning "/9j/4AAQ..." from curl | base64) for image content blocks. [1][2]
Official docs:
- Vision / base64 image example (Messages API): https://docs.anthropic.com/en/docs/build-with-claude/vision. [1]
- Messages examples (vision section): https://docs.anthropic.com/en/api/messages-examples (shows the base64 image block). [2]
Minimal example (JSON) — use standard base64 (note the leading "/9j/..." typical of JPEG standard Base64):
{
"model": "claude-sonnet-4-20250514",
"messages": [
{
"role": "user",
"content": [
{
"type": "image",
"source": {
"type": "base64",
"media_type": "image/jpeg",
"data": "/9j/4AAQSkZJRgABAQAAAQABAAD/2wCEAAkGBxISEhUTEhIVFhUVFRUVFRUVFRUVFRUWFhUVFRUYHSggGBolGxUVITEhJSkrLi4uFx8zODMsNygtLisBCgoKDg0OGhAQGi0lHyUtLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLf/AABEIA..."
}
},
{ "type": "text", "text": "Describe this image." }
]
}
]
}
(Generate standard base64 with e.g. linux: curl "$URL" | base64). [1][2]
Normalize URL-safe base64 to standard base64 for Anthropic image blocks
Anthropic’s Messages API requires standard Base64 (not URL-safe). ResourceContent.data may be URL-safe — normalize to avoid sporadic provider errors.
File: src/draive/anthropic/messages.py (lines 488–495)
image_block: ImageBlockParam = {
"type": "image",
"source": {
"type": "base64",
- "media_type": cast(Any, part.mime_type),
- "data": part.data, # ResourceContent.data is already base64-encoded
+ "media_type": cast(Any, part.mime_type),
+ # Normalize to standard base64 (padding + alphabet) to satisfy API expectations
+ "data": _normalize_base64(part.data),
},
}Add (module-level):
def _normalize_base64(data: str) -> str:
std = data.replace("-", "+").replace("_", "/")
return std + "=" * (-len(std) % 4)Verified with Anthropic Messages API vision/base64 examples (docs show standard base64 like "/9j/...").
🤖 Prompt for AI Agents
In src/draive/anthropic/messages.py around lines 488-495, the ImageBlockParam
uses part.data directly but Anthropic requires standard (non-URL-safe) Base64;
normalize URL-safe base64 to standard form before assigning to "data". Add a
module-level helper _normalize_base64(data: str) that replaces "-" with "+" and
"_" with "/", then pads with "=" to a multiple of 4, and call it when building
image_block so the value written to "data" is the normalized base64 string.
| def _expand_uri(self, values: Mapping[str, Any], provided: Set[str]) -> str: | ||
| template: str = self.declaration.template_uri | ||
| resolved: list[str] = [] | ||
|
|
||
| for part in re.split(r"(\{[^}]+\})", template): | ||
| if not part or not part.startswith("{"): | ||
| resolved.append(part) | ||
| continue | ||
|
|
||
| # {/var} | ||
| m = re.fullmatch(r"\{/([^}]+)\}", part) | ||
| if m: | ||
| name = m.group(1) | ||
| seg = self._encode_path_segment(values.get(name)) | ||
| resolved.append("/" + seg if seg else "") | ||
| continue | ||
|
|
||
| # {?a,b} | ||
| m = re.fullmatch(r"\{\?([^}]+)\}", part) | ||
| if m: | ||
| names = [n.strip() for n in m.group(1).split(",") if n.strip()] | ||
| resolved.append(self._expand_query(names, "".join(resolved), values, provided)) | ||
| continue | ||
|
|
||
| # {#var} | ||
| m = re.fullmatch(r"\{#([^}]+)\}", part) | ||
| if m: | ||
| name = m.group(1) | ||
| if name in provided: | ||
| frag = self._encode_fragment(values.get(name)) | ||
| resolved.append("#" + frag if frag else "") | ||
| else: | ||
| resolved.append("") | ||
| continue | ||
|
|
||
| # {var} | ||
| m = re.fullmatch(r"\{([^}]+)\}", part) | ||
| if m: | ||
| name = m.group(1) | ||
| if name.startswith("?") or name.startswith("#"): | ||
| resolved.append("") | ||
| else: | ||
| resolved.append(self._encode_path_segment(values.get(name))) | ||
| continue | ||
|
|
||
| resolved.append(part) | ||
|
|
||
| return "".join(resolved) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Query expansion should ignore unknown names silently (OK) but consider allowing defaults.
Currently only explicitly provided kwargs affect {?…}. If defaults should appear in URI, include them by checking values instead of provided. Confirm desired behavior.
🤖 Prompt for AI Agents
In src/draive/resources/template.py around lines 197 to 245, the {?a,b} branch
currently includes only names present in the provided set when building the
query string; change that logic so it also treats a name as present if values
contains a non-None/non-empty value for that key (i.e., check values.get(name)
in addition to provided) so default values in the values mapping are included in
the expanded query; ensure you treat falsy-but-valid values appropriately
(decide whether empty string should be included) and keep existing behavior of
ignoring entirely missing names.
| def test_returns_all_tags(): | ||
| tags = list(MultimodalContent.of("<test>Lorem ipsum</test><test>Other</test>").tags("test")) | ||
|
|
||
| assert MultimodalTagElement.parse_first( | ||
| "test", | ||
| content=MultimodalContent.of( | ||
| 'Lorem<test key="value" next="more" spaces="s p a ce" >Lorem ipsum</test>ipsum' | ||
| ), | ||
| ) == MultimodalTagElement( | ||
| name="test", | ||
| content=MultimodalContent.of("Lorem ipsum"), | ||
| attributes={"key": "value", "next": "more", "spaces": "s p a ce"}, | ||
| ) | ||
| assert len(tags) == 2 | ||
| assert tags[0].name == "test" | ||
| assert tags[0].content == MultimodalContent.of("Lorem ipsum") | ||
| assert tags[1].name == "test" | ||
| assert tags[1].content == MultimodalContent.of("Other") | ||
|
|
||
| assert MultimodalTagElement.parse_first( | ||
| "test", | ||
| content=MultimodalContent.of( | ||
| 'Lorem<test key="value" escaping="\\"escape" spaces="s p a ce" >Lorem ipsum</test>ipsum' | ||
| ), | ||
| ) == MultimodalTagElement( | ||
| name="test", | ||
| content=MultimodalContent.of("Lorem ipsum"), | ||
| attributes={"key": "value", "escaping": '"escape', "spaces": "s p a ce"}, | ||
| tags = list( | ||
| MultimodalContent.of( | ||
| "<test>Lorem", | ||
| ResourceReference.of(uri="http://image1", mime_type="image/png"), | ||
| "ipsum</test><test>Other</test>", | ||
| ).tags("test") | ||
| ) | ||
| assert MultimodalTagElement.parse_first( | ||
| "test", | ||
| content=MultimodalContent.of( | ||
| MediaReference.of("http://image", media="image/png"), | ||
| 'Lorem<test key="value">Lorem', | ||
| MediaReference.of("http://image", media="image/png"), | ||
| "ipsum</test>ipsum", | ||
| MediaReference.of("http://image", media="image/png"), | ||
| ), | ||
| ) == MultimodalTagElement( | ||
| name="test", | ||
| content=MultimodalContent.of( | ||
| "Lorem", | ||
| MediaReference.of("http://image", media="image/png"), | ||
| "ipsum", | ||
| ), | ||
| attributes={"key": "value"}, | ||
|
|
||
| assert len(tags) == 2 | ||
| assert tags[0].name == "test" | ||
| assert tags[0].content == MultimodalContent.of( | ||
| "Lorem", | ||
| ResourceReference.of(uri="http://image1", mime_type="image/png"), | ||
| "ipsum", | ||
| ) | ||
| assert tags[1].name == "test" | ||
| assert tags[1].content == MultimodalContent.of("Other") | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Parametrize to reduce repetition.
Several blocks assert similar properties across multiple inputs. Parametrization will keep the suite concise without losing clarity.
Also applies to: 267-305, 306-331
| def test_returns_unchanged_without_tag(): | ||
| assert MultimodalTagElement.replace( | ||
| assert MultimodalContent.of("Lorem ipsum").replacing_tag( | ||
| "test", | ||
| content=MultimodalContent.of("Lorem ipsum"), | ||
| replacement="replaced", | ||
| ) == MultimodalContent.of("Lorem ipsum") | ||
|
|
||
| assert MultimodalTagElement.replace( | ||
| assert MultimodalContent.of("Lorem ipsum").replacing_tag( | ||
| "test", | ||
| content=MultimodalContent.of("Lorem ipsum"), | ||
| replacement="replaced", | ||
| strip_tags=True, | ||
| ) == MultimodalContent.of("Lorem ipsum") | ||
|
|
||
| assert MultimodalTagElement.replace( | ||
| assert MultimodalContent.of( | ||
| "Lorem", | ||
| ResourceReference.of(uri="http://image", mime_type="image/png"), | ||
| "ipsum", | ||
| ).replacing_tag( | ||
| "test", | ||
| content=MultimodalContent.of( | ||
| "Lorem", | ||
| MediaReference.of("http://image", media="image/png"), | ||
| "ipsum", | ||
| ), | ||
| replacement="replaced", | ||
| ) == MultimodalContent.of( | ||
| "Lorem", | ||
| MediaReference.of("http://image", media="image/png"), | ||
| ResourceReference.of(uri="http://image", mime_type="image/png"), | ||
| "ipsum", |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Reduce duplication via parametrization/fixtures.
Many cases differ only in inputs/flags. Consider pytest.mark.parametrize and a small fixture for png_ref to make tests shorter and easier to maintain.
Example fixture (in tests/conftest.py):
import pytest
from draive.resources import ResourceReference
@pytest.fixture
def png_ref():
return ResourceReference.of(uri="http://image", mime_type="image/png")Also applies to: 68-78, 286-298, 379-391, 405-416
🤖 Prompt for AI Agents
In tests/test_tags_replacement.py around lines 24 to 46, the test repeats
similar cases with only different inputs/flags and an inline PNG
ResourceReference; replace the duplicated assertions by parametrizing the inputs
and expected outputs with pytest.mark.parametrize and move the common
ResourceReference creation into a png_ref fixture (e.g., in tests/conftest.py)
so each case only supplies the varying arguments (tag, replacement, strip_tags
flag, and expected MultimodalContent) and the fixture provides the png_ref to
use in parameterized cases, reducing duplication and improving maintainability.
KaQuMiQ
force-pushed
the
feature/content
branch
from
0adde41 to
7973093
Compare
There was a problem hiding this comment.
Actionable comments posted: 29
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (32)
Makefile (2)
33-45: Version check skips downgrades and is non-portable (sort -V)Current logic only updates when behind and relies on GNU sort -V (missing on macOS/BSD). Compare for exact match and always (re)install the pinned version when mismatched.
Apply:
- CURRENT_VERSION=$$(uv --version | head -n1 | cut -d" " -f2); \ - if [ "$$(printf "%s\n%s" "$(UV_VERSION)" "$$CURRENT_VERSION" | sort -V | head -n1)" != "$(UV_VERSION)" ]; then \ - echo '...updating uv...'; \ - curl -fLsS https://astral.sh/uv/install.sh | sh; \ + CURRENT_VERSION=$$(uv --version | awk 'NR==1{print $$2}'); \ + if [ "$$CURRENT_VERSION" != "$(UV_VERSION)" ]; then \ + echo '...installing pinned uv $(UV_VERSION)...'; \ + curl -fLsS https://astral.sh/uv/install.sh | sh -s -- --version $(UV_VERSION) --force; \ if [ $$? -ne 0 ]; then \ - echo "...updating uv failed!"; \ + echo "...installing pinned uv failed!"; \ exit 1; \ fi; \ else \ echo '...uv version is up-to-date!'; \ fi; \
21-31: Ensure uv is discoverable on PATH after installOn fresh environments, ~/.local/bin (and ~/.cargo/bin) may not be on PATH, causing the subsequent version check to miss a just-installed uv.
Add a PATH guard before checks:
uv_check: @echo 'Checking uv version...' + # Ensure typical install locations are on PATH for this session + @export PATH="$$HOME/.local/bin:$$HOME/.cargo/bin:$$PATH"; \ + # Install if not presentsrc/draive/parameters/model.py (3)
955-966: NameError in dataclass branch of _data_str — wrong variable used.
parametrized_datais undefined in this scope; should read fromdataclass.Apply:
- element: str = _data_str( - getattr(parametrized_data, field.name), + element: str = _data_str( + getattr(dataclass, field.name),
277-286: Schema now requires additionalProperties: False — emit it for DataModel specs.
ParameterObjectSpecificationwas tightened; producers must setadditionalProperties: False. Add it when properties exist.Apply:
if parameters_specification: data_type.__PARAMETERS_SPECIFICATION__ = { # pyright: ignore[reportAttributeAccessIssue] "type": "object", "properties": parameters_specification, + "additionalProperties": False, "required": parameters_specification_required, }
347-350: Exception message formatting as tuple.Use f-string to avoid
RuntimeError(args=("msg", subclass)).Apply:
- raise RuntimeError("Invalid type parametrization for %s", subclass) + raise RuntimeError(f"Invalid type parametrization for {subclass}")src/draive/evaluators/fluency.py (1)
67-75: Normalize RESULT value and harden parsing.Trim/lower the tag payload to reduce LLM-format fragility; keep meta unchanged. Also consider a custom exception (e.g., InvalidEvaluatorResultError) instead of ValueError per guidelines.
Apply:
- if result := completion.tag("RESULT"): - return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), - meta={"comment": completion.to_str()}, - ) + if result := completion.tag("RESULT"): + value = result.content.to_str().strip().lower() + return EvaluationScore.of( + cast(EvaluationScoreValue, value), + meta={"comment": completion.to_str()}, + )Additionally: define/raise a dedicated exception for parse failures (not shown here to avoid touching non-changed lines).
src/draive/evaluators/safety.py (1)
99-107: Strip/lower RESULT and use a typed exception.Mirror resilience across evaluators and align with custom-exception guideline.
- if result := completion.tag("RESULT"): - return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), - meta={"comment": completion.to_str()}, - ) + if result := completion.tag("RESULT"): + value = result.content.to_str().strip().lower() + return EvaluationScore.of( + cast(EvaluationScoreValue, value), + meta={"comment": completion.to_str()}, + )Also consider replacing
ValueErrorwith a domain-specific error (e.g.,InvalidEvaluatorResultError).src/draive/evaluators/creativity.py (1)
101-109: Result normalization + custom exception.Same robustness tweak as other evaluators.
- if result := completion.tag("RESULT"): - return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), - meta={"comment": completion.to_str()}, - ) + if result := completion.tag("RESULT"): + value = result.content.to_str().strip().lower() + return EvaluationScore.of( + cast(EvaluationScoreValue, value), + meta={"comment": completion.to_str()}, + )Also prefer a dedicated parse error over
ValueError.src/draive/evaluators/readability.py (1)
72-80: Harden parsing and add a NumPy-style docstring.
- Normalize the RESULT; prefer a custom exception type.
- This public API lacks a NumPy-style docstring; please add Parameters/Returns/Raises.
- if result := completion.tag("RESULT"): - return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), - meta={"comment": completion.to_str()}, - ) + if result := completion.tag("RESULT"): + value = result.content.to_str().strip().lower() + return EvaluationScore.of( + cast(EvaluationScoreValue, value), + meta={"comment": completion.to_str()}, + )Example docstring to add above the function (outside this hunk):
""" Evaluate content readability. Parameters ---------- evaluated : Multimodal Content to assess. guidelines : str | None, optional Extra evaluator guidance. Returns ------- EvaluationScore Categorical readability score with meta. Raises ------ InvalidEvaluatorResultError When RESULT tag cannot be parsed. """src/draive/evaluators/conciseness.py (1)
83-91: Normalize RESULT and document the public API.
- Use
.strip().lower()before casting.- Prefer a typed exception over
ValueError.- Add a NumPy-style docstring to
conciseness_evaluator(public API).- if result := completion.tag("RESULT"): - return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), - meta={"comment": completion.to_str()}, - ) + if result := completion.tag("RESULT"): + value = result.content.to_str().strip().lower() + return EvaluationScore.of( + cast(EvaluationScoreValue, value), + meta={"comment": completion.to_str()}, + )src/draive/evaluators/groundedness.py (1)
82-90: RESULT parsing resilience and docstring.
- Normalize and cast the parsed value.
- Replace generic
ValueErrorwith a custom error.- Add a NumPy-style docstring to this public evaluator.
- if result := completion.tag("RESULT"): - return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), - meta={"comment": completion.to_str()}, - ) + if result := completion.tag("RESULT"): + value = result.content.to_str().strip().lower() + return EvaluationScore.of( + cast(EvaluationScoreValue, value), + meta={"comment": completion.to_str()}, + )src/draive/generation/audio/default.py (1)
18-30: Add NumPy-style docstring and minimal debug logs.Public API needs a docstring per guidelines; also log around generation without leaking details.
async def generate_audio( *, instructions: ResolveableInstructions, input: MultimodalContent, # noqa: A002 **extra: Any, ) -> ResourceContent | ResourceReference: + """ + Generate an audio resource from multimodal input. + + Parameters + ---------- + instructions : ResolveableInstructions + Instruction handle to resolve before generation. + input : MultimodalContent + Multimodal input context for audio generation. + **extra : Any + Provider-specific options forwarded to the model. + + Returns + ------- + ResourceContent | ResourceReference + First audio resource produced by the model. + + Raises + ------ + ValueError + When no audio resource is present in the output. + """ async with ctx.scope("generate_audio"): + ctx.log_debug("Starting audio generation") result: ModelOutput = await GenerativeModel.completion( instructions=await InstructionsRepository.resolve(instructions), context=[ModelInput.of(input)], output="audio", **extra, ) + ctx.log_debug("Model output received; extracting audio")src/draive/evaluators/expectations.py (2)
76-80: Trim RESULT payload before casting.Model outputs often include stray whitespace/newlines; strip to keep rating parsing robust.
- return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), - meta={"comment": completion.to_str()}, - ) + return EvaluationScore.of( + cast(EvaluationScoreValue, result.content.to_str().strip()), + meta={"comment": completion.to_str()}, + )
41-48: Add a concise NumPy-style docstring to the public evaluator.Public functions must include Parameters/Returns/Raises.
@evaluator(name="expectations") async def expectations_evaluator( evaluated: Multimodal, /, *, expectations: Multimodal, guidelines: str | None = None, ) -> EvaluationScore: + """ + Evaluate expectation fulfilment of content against explicit expectations. + + Parameters + ---------- + evaluated : Multimodal + Content to rate. + expectations : Multimodal + Expected criteria/content to compare against. + guidelines : str | None + Optional additional guidance for the evaluator. + + Returns + ------- + EvaluationScore + Categorical rating with meta including the full completion. + + Raises + ------ + ValueError + When RESULT tag is missing from the model completion. + """src/draive/evaluators/completeness.py (1)
110-117: Trim RESULT before casting to the enum-like type.Avoid mismatches from trailing whitespace/newlines.
- if result := completion.tag("RESULT"): - return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), - meta={"comment": completion.to_str()}, - ) + if result := completion.tag("RESULT"): + return EvaluationScore.of( + cast(EvaluationScoreValue, result.content.to_str().strip()), + meta={"comment": completion.to_str()}, + )src/draive/evaluators/truthfulness.py (2)
82-89: Trim RESULT before casting to the rating type.Prevents rating mismatches due to whitespace.
- if result := completion.tag("RESULT"): - return EvaluationScore.of( - cast(EvaluationScoreValue, result.content.to_str()), - meta={"comment": completion.to_str()}, - ) + if result := completion.tag("RESULT"): + return EvaluationScore.of( + cast(EvaluationScoreValue, result.content.to_str().strip()), + meta={"comment": completion.to_str()}, + )
47-54: Add NumPy-style docstring to the public evaluator.Aligns with project docstring requirements.
@evaluator(name="truthfulness") async def truthfulness_evaluator( evaluated: Multimodal, /, *, reference: Multimodal, guidelines: str | None = None, ) -> EvaluationScore: + """ + Evaluate truthfulness (factual accuracy) of content against a reference. + + Parameters + ---------- + evaluated : Multimodal + Content to evaluate. + reference : Multimodal + Reference facts/material. + guidelines : str | None + Optional evaluator guidance. + + Returns + ------- + EvaluationScore + Categorical truthfulness rating with explanation in meta. + + Raises + ------ + ValueError + When RESULT tag is missing from the completion. + """src/draive/openai/images.py (1)
43-61: Fix double-encoding and raise typed errors; propagate MIME type when available
ResourceContent.of(b64data, ...)encodes an already base64 string again. Decode to bytes first, then call.of(bytes, ...).- Prefer
ModelOutputInvalidwith provider/model context overValueError.- Try to propagate MIME type from the SDK when present; fallback to
"image/png".+from base64 import b64decode @@ -from draive.models import InstructionsRepository, ResolveableInstructions +from draive.models import InstructionsRepository, ResolveableInstructions, ModelOutputInvalid @@ - if response.data is None: - raise ValueError("Invalid OpenAI response - missing image content") + if not response.data: + raise ModelOutputInvalid( + provider="openai", + model=generation_config.model, + reason="Missing image content in ImagesResponse", + ) @@ - image: Image = response.data[0] - if url := image.url: - return ResourceReference.of( - url, - mime_type="image/png", - ) - elif b64data := image.b64_json: - return ResourceContent.of( - b64data, - mime_type="image/png", - ) + image: Image = response.data[0] + mime: str = ( + getattr(image, "mime_type", None) + or getattr(image, "content_type", None) + or "image/png" + ) + if url := image.url: + # If SDK exposes a MIME type, propagate it; fallback to PNG. + return ResourceReference.of(url, mime_type=mime) + elif b64data := image.b64_json: + try: + raw = b64decode(b64data) + except Exception as exc: + raise ModelOutputInvalid( + provider="openai", + model=generation_config.model, + reason=f"Invalid base64 image data: {exc}", + ) from exc + return ResourceContent.of(raw, mime_type=mime) else: - raise ValueError("Invalid OpenAI response - missing image content") + raise ModelOutputInvalid( + provider="openai", + model=generation_config.model, + reason="Image payload missing both url and b64_json", + )src/draive/generation/image/state.py (3)
14-14: Add NumPy-style docstrings to the public state class.Public APIs in src/ require NumPy-style docstrings.
class ImageGeneration(State): + """ + Image generation state facade. + + Notes + ----- + Provides a `generate` statemethod that delegates to the configured + `generating` implementation (default: `generate_image`). + """
35-46: Document the publicgenerateAPI.Add a NumPy-style docstring describing parameters, return type, and behavior.
@statemethod async def generate( self, *, instructions: ResolveableInstructions = "", input: Multimodal, # noqa: A002 **extra: Any, ) -> ResourceContent | ResourceReference: + """ + Generate an image from multimodal input. + + Parameters + ---------- + instructions : ResolveableInstructions, optional + Optional provider/model instructions to steer generation. + input : Multimodal + Input content (text and/or resources) to condition the image. + **extra : Any + Provider-specific options forwarded to the underlying generator. + + Returns + ------- + ResourceContent | ResourceReference + Generated image content (inline) or a reference (uri). + """ return await self.generating( instructions=instructions, input=MultimodalContent.of(input), **extra, )
1-1: Optionally mark the facade class as final.If not intended for subclassing, annotate as
finalto communicate API intent.-from typing import Any, overload +from typing import Any, overload, final @@ -class ImageGeneration(State): +@final +class ImageGeneration(State): ...src/draive/evaluators/similarity.py (3)
49-56: Add NumPy-style docstring tosimilarity_evaluator.Public API requires docstrings.
@evaluator(name="similarity") async def similarity_evaluator( evaluated: Multimodal, /, *, reference: Multimodal, guidelines: str | None = None, ) -> EvaluationScore: + """ + Compare two multimodal inputs using an LLM-guided similarity rubric. + + Parameters + ---------- + evaluated : Multimodal + Evaluated content. + reference : Multimodal + Reference content. + guidelines : str or None, optional + Additional rubric guidance injected into the instruction. + + Returns + ------- + EvaluationScore + Discrete similarity rating with explanation in metadata. + """
94-101: Add NumPy-style docstring totext_vector_similarity_evaluator.@evaluator(name="text_vector_similarity") async def text_vector_similarity_evaluator( evaluated: str, /, *, reference: str, ) -> float: + """ + Compute cosine similarity between text embeddings. + + Parameters + ---------- + evaluated : str + Evaluated text. + reference : str + Reference text. + + Returns + ------- + float + Cosine similarity in [0, 1]. + """
132-139: Keep evaluated/reference ordering consistent (nit).Symmetric metric, but align ordering with parameter names for clarity.
- embedding: Sequence[Embedded[bytes]] = await ImageEmbedding.embed( - [reference_data, evaluated_data] - ) + embedding: Sequence[Embedded[bytes]] = await ImageEmbedding.embed( + [evaluated_data, reference_data] + ) return vector_similarity_score( - value_vector=embedding[0].vector, - reference_vector=embedding[1].vector, + value_vector=embedding[0].vector, # evaluated + reference_vector=embedding[1].vector, # reference )src/draive/ollama/chat.py (1)
214-215: Fix isinstance with union types.Pass a tuple, not a PEP 604 union.
- elif isinstance(block, ModelReasoning | ModelToolRequest): + elif isinstance(block, (ModelReasoning, ModelToolRequest)): yield blocksrc/draive/mistral/completions.py (1)
359-366: Guard None content for ModelInput.Avoid attribute access on None.
- user_content = element.content - yield cast( - MessagesTypedDict, - { - "role": "user", - "content": list(_content_chunks(user_content.parts)), - }, - ) + if content := element.content: + yield cast( + MessagesTypedDict, + { + "role": "user", + "content": list(_content_chunks(content.parts)), + }, + )src/draive/helpers/volatile_vector_index.py (1)
73-77: Fix isinstance(..., bytes | bytearray).Use tuple to avoid runtime TypeError.
- elif all(isinstance(value, bytes | bytearray) for value in selected_values): + elif all(isinstance(value, (bytes, bytearray)) for value in selected_values): embedded_values = await ImageEmbedding.embed( cast(list[bytes], selected_values), **extra, )src/draive/openai/realtime.py (2)
570-579: Fix dict mutation during iteration and preserve shared mapping instance.
- Deleting keys while iterating
current_items.keys()can raise RuntimeError.- Reassigning
current_items = {}breaks the closure used by readers; clear in-place instead.Apply:
- for item_id in current_items.keys(): + for item_id in list(current_items.keys()): try: await connection.conversation.item.delete(item_id=item_id) - del current_items[item_id] + current_items.pop(item_id, None)- await _reset_context( + await _reset_context( (await memory.recall()).context, current_items=current_items, connection=connection, ) - current_items = {} + # keep current_items object; _reset_context repopulates itAlso applies to: 431-437
858-866: Handle assistant audio in memory;audio_formatis currently unused.Assistant message parts with audio are ignored; store them as
ResourceContentlike user audio.Apply:
def _assistant_content_to_multimodal( content: Sequence[AssistantContent], /, audio_format: str, ) -> MultimodalContent: parts: list[MultimodalContentPart] = [] for part in content: if part.text: parts.append(TextContent.of(part.text)) + if getattr(part, "audio", None): + parts.append( + ResourceContent.of( + b64decode(part.audio), + mime_type=audio_format, + ) + ) + if getattr(part, "transcript", None): + parts.append(TextContent.of(part.transcript, meta={"transcript": True})) return MultimodalContent.of(*parts)src/draive/openai/responses.py (1)
307-323: Refusal handling may misclassify non-text parts.The else-branch assumes a refusal shape; consider matching the explicit refusal type if the SDK exposes it to avoid false positives.
src/draive/resources/template.py (2)
416-416: Type mime_type as MimeType | None.Aligns with public types and stricter typing.
- mime_type: str | None = None, + mime_type: MimeType | None = None,
413-421: Add a NumPy-style docstring to resource() decorator.Top-level public API; document parameters/returns.
def resource[**Args]( *, uri_template: str, - mime_type: str | None = None, + mime_type: MimeType | None = None, name: str | None = None, description: str | None = None, availability_check: ResourceAvailabilityCheck | None = None, meta: Meta | MetaValues | None = None, ) -> Callable[ [Callable[Args, Coroutine[None, None, Sequence[ResourceReference] | ResourceContent]]], ResourceTemplate[Args], ]: + """ + Decorator that declares a resource template. + + Parameters + ---------- + uri_template : str + RFC 6570-like template used to build/match URIs. + mime_type : MimeType or None, optional + Expected MIME (advisory). + name : str or None, optional + Display name (defaults to function name). + description : str or None, optional + Optional description. + availability_check : ResourceAvailabilityCheck or None, optional + Callable indicating whether the resource is currently available. + meta : Meta or Mapping, optional + Additional metadata. + + Returns + ------- + Callable + Wrapper producing a ResourceTemplate for the decorated function. + """
|
|
||
| ## Framework Foundation (Haiway) | ||
|
|
||
| - Draive is built on top of Haiway (state, context, observability, config). See: https://github.com/miquido/haiway |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Replace bare URL with Markdown link.
Avoid MD034.
-- Draive is built on top of Haiway (state, context, observability, config). See: https://github.com/miquido/haiway
+- Draive is built on top of Haiway (state, context, observability, config). See: [miquido/haiway](https://github.com/miquido/haiway)📝 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.
| - Draive is built on top of Haiway (state, context, observability, config). See: https://github.com/miquido/haiway | |
| - Draive is built on top of Haiway (state, context, observability, config). See: [miquido/haiway](https://github.com/miquido/haiway) |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
15-15: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In AGENTS.md around line 15, replace the bare URL with a Markdown link to avoid
MD034 by changing the sentence to include a link text (e.g., "Haiway") pointing
to https://github.com/miquido/haiway; update the line so the URL is not
displayed raw but used as the target of the Markdown link.
| - Access active state through `haiway.ctx` inside async scopes (`ctx.scope(...)`). | ||
| - Public state methods that dispatch on the active instance should use `@statemethod` (see `GenerativeModel`). | ||
|
|
||
| #### Examples: |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove trailing colon in heading.
Fix MD026.
-#### Examples:
+#### Examples📝 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.
| #### Examples: | |
| #### Examples |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
58-58: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🤖 Prompt for AI Agents
In AGENTS.md around line 58 the heading "#### Examples:" ends with a trailing
colon which triggers MD026; remove the trailing colon so the heading reads "####
Examples" (no punctuation) to comply with the rule.
| ``` | ||
|
|
||
| State with statemethod helper: | ||
| ```python |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add blank lines around fenced code block.
Fix MD031 (before/after the fence).
-State with statemethod helper:
-```python
+State with statemethod helper:
+
+```python
...
-```
+```
+🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
83-83: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
In AGENTS.md around line 83, the fenced Python code block is missing a blank
line before and after the ```python fence, which triggers MD031; add one blank
line immediately before the opening ```python fence and one blank line
immediately after the closing ``` fence so the code block is separated from
surrounding text.
| @property | ||
| def reference(self) -> ResourceReference: | ||
| if isinstance(self.content, ResourceContent): | ||
| return ResourceReference( | ||
| uri=self.uri, | ||
| mime_type=self.content.mime_type, | ||
| meta=self.meta, | ||
| ) | ||
|
|
||
| case content: | ||
| return MultimodalContent.of(*[element.to_multimodal() for element in content]) | ||
| else: | ||
| return ResourceReference( | ||
| uri=self.uri, | ||
| mime_type=None, | ||
| meta=self.meta, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Property method should be named to_reference() for clarity
The property reference creates a new ResourceReference object. Consider renaming to to_reference() as a regular method to make the conversion operation more explicit.
- @property
- def reference(self) -> ResourceReference:
+ def to_reference(self) -> ResourceReference:
+ """
+ Convert this Resource to a ResourceReference.
+
+ Returns
+ -------
+ ResourceReference
+ A reference to this resource with appropriate metadata.
+ """
if isinstance(self.content, ResourceContent):
return ResourceReference(
uri=self.uri,
mime_type=self.content.mime_type,
meta=self.meta,
)
else:
return ResourceReference(
uri=self.uri,
mime_type=None,
meta=self.meta,
)📝 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.
| @property | |
| def reference(self) -> ResourceReference: | |
| if isinstance(self.content, ResourceContent): | |
| return ResourceReference( | |
| uri=self.uri, | |
| mime_type=self.content.mime_type, | |
| meta=self.meta, | |
| ) | |
| case content: | |
| return MultimodalContent.of(*[element.to_multimodal() for element in content]) | |
| else: | |
| return ResourceReference( | |
| uri=self.uri, | |
| mime_type=None, | |
| meta=self.meta, | |
| ) | |
| def to_reference(self) -> ResourceReference: | |
| """ | |
| Convert this Resource to a ResourceReference. | |
| Returns | |
| ------- | |
| ResourceReference | |
| A reference to this resource with appropriate metadata. | |
| """ | |
| if isinstance(self.content, ResourceContent): | |
| return ResourceReference( | |
| uri=self.uri, | |
| mime_type=self.content.mime_type, | |
| meta=self.meta, | |
| ) | |
| else: | |
| return ResourceReference( | |
| uri=self.uri, | |
| mime_type=None, | |
| meta=self.meta, | |
| ) |
🤖 Prompt for AI Agents
In src/draive/resources/types.py around lines 308 to 323, the property named
`reference` should be converted into an explicit method `to_reference(self) ->
ResourceReference` to make the conversion operation clear: remove the @property
decorator, rename the function to `to_reference`, keep the same logic and return
value, and update the type hint accordingly; then search the codebase for any
usages of `.reference` and replace them with `.to_reference()` (adjust tests and
imports if needed) to ensure callers use the new method form.
| @@ -1,11 +1,11 @@ | |||
| from draive import MediaContent, MediaReference, MetaContent, MultimodalContent, TextContent | |||
| from draive import ArtifactContent, DataModel, MultimodalContent, ResourceReference, TextContent | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer explicit submodule imports for public surface consistency.
Import ArtifactContent from draive.multimodal and ResourceReference from draive.resources (other tests already do this). Keeps API usage explicit and avoids brittle root re-exports.
-from draive import ArtifactContent, DataModel, MultimodalContent, ResourceReference, TextContent
+from draive import DataModel, MultimodalContent, TextContent
+from draive.multimodal import ArtifactContent
+from draive.resources import ResourceReference📝 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.
| from draive import ArtifactContent, DataModel, MultimodalContent, ResourceReference, TextContent | |
| from draive import DataModel, MultimodalContent, TextContent | |
| from draive.multimodal import ArtifactContent | |
| from draive.resources import ResourceReference |
🤖 Prompt for AI Agents
In tests/test_multimodal.py around lines 1 to 1, the test currently imports
ArtifactContent and ResourceReference from the package root which is
inconsistent with other tests; change the imports so ArtifactContent comes from
draive.multimodal and ResourceReference comes from draive.resources, keeping
DataModel, MultimodalContent, and TextContent imports as-is, to make the public
surface explicit and avoid relying on root re-exports.
| # Empty strings now create a single TextContent with empty text | ||
| result = MultimodalContent.of("", "", "") | ||
| assert len(result.parts) == 1 | ||
| assert result.parts[0].text == "" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Assert structure without over-coupling to exact positions.
These checks hard-code part indices. Consider asserting via predicates (types/meta) or slicing groups to reduce brittleness if merge logic evolves.
Also applies to: 80-106
🤖 Prompt for AI Agents
In tests/test_multimodal.py around lines 39 to 43, the assertions rely on exact
part indices (assert len(result.parts) == 1 and result.parts[0].text == "")
which is brittle; change them to assert that there exists a TextContent part
with empty text (e.g., filter or any(...) over result.parts by type/meta and
text) and that overall part count matches expected grouping semantics; apply the
same predicate-based / membership-style assertions for the similar checks at
lines 80-106 to avoid coupling to exact positions.
7973093 to
7c1ee2b
Compare
KaQuMiQ
force-pushed
the
feature/content
branch
from
7c1ee2b to
b895179
Compare
1 participant
No description provided.