Conversation
|
Warning Rate limit exceeded@KaQuMiQ has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 26 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
WalkthroughThis PR updates build/config (Makefile: remove .env include; UV_VERSION 0.8.22) and bumps package/dependency versions (pyproject: 0.86.0; haiway ~=0.34.0). It replaces OpenAI/vLLM missing-value sentinels from NOT_GIVEN/NotGiven to Omit/omit across modules. Parameters subsystem undergoes major refactor: new ParametersJSONEncoder; validation API changes (validated→validate; ParameterValidator signatures); specification dispatch via AttributeAnnotation types; DataModel redesigned around FIELDS, serialization, and generics handling. MCP client switches to ToolParametersSpecification. ResourceTemplate gains URI parameter coercion per ParameterSpecification. Tests updated to match new specification/streaming behaviors. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/draive/parameters/function.py (2)
282-285: Fix TypeError formatting (currently creates a tuple message).Use an f-string or % formatting to avoid a tuple in the exception args.
- raise TypeError( - "Untyped argument %s", - parameter.name, - ) + raise TypeError(f"Untyped argument {parameter.name}")
281-294: Preserve Annotated metadata by adding include_extras=True
Replace the lone call in src/draive/parameters/function.py (line 187)- type_hints: Mapping[str, Any] = get_type_hints(function) + type_hints: Mapping[str, Any] = get_type_hints(function, include_extras=True)src/draive/resources/template.py (1)
279-307: Fix parameter extraction regex: wrong group order, missing escaping, and single-segment slash capture.Current approach can mis-map values when {/var} and {var} are interleaved, treats {/var} as a single segment, and doesn’t escape literal regex metacharacters (e.g., dots). Build the regex in one pass, preserve parameter order, escape literals, and capture multi-segment slash expansions.
Apply this diff:
- def _extract_path_parameters_with_slash( - self, - actual_path: str, - ) -> dict[str, str]: - params: dict[str, str] = {} - slash_params: list[Any] = re.findall(r"\{/([^}]+)\}", self.declaration.template_uri) - path_template: str = self._extract_path_template() - - # Create regex pattern - template_for_regex: str = path_template - for param in slash_params: - template_for_regex = template_for_regex.replace(f"{{/{param}}}", "/([^/]+)") - - # Handle regular {var} patterns - regular_params: list[Any] = re.findall(r"\{([^}]+)\}", template_for_regex) - for param in regular_params: - if not param.startswith("/"): # Skip already handled slash patterns - template_for_regex = template_for_regex.replace(f"{{{param}}}", "([^/]+)") - - # Match against the actual path - match: re.Match[str] | None = re.match(f"^{template_for_regex}$", actual_path) - if match: - all_params: list[Any] = slash_params + [ - p for p in regular_params if not p.startswith("/") - ] - for param, value in zip(all_params, match.groups(), strict=False): - params[param] = value - - return params + def _extract_path_parameters_with_slash( + self, + actual_path: str, + ) -> dict[str, str]: + # Build a safe regex and preserve parameter order + path_template: str = self._extract_path_template() + pattern_parts: list[str] = [] + param_names: list[str] = [] + + idx = 0 + for m in re.finditer(r"\{(/)?([^}]+)\}", path_template): + # Literal part before the placeholder + if m.start() > idx: + pattern_parts.append(re.escape(path_template[idx : m.start()])) + + slash_prefix, name = m.groups() + param_names.append(name) + if slash_prefix: + # Capture one-or-more slash-delimited segments + pattern_parts.append(r"((?:/[^/?#]+)+)") + else: + # Capture a single path segment + pattern_parts.append(r"([^/?#]+)") + + idx = m.end() + + # Trailing literal + if idx < len(path_template): + pattern_parts.append(re.escape(path_template[idx:])) + + pattern = "^" + "".join(pattern_parts) + "$" + m = re.match(pattern, actual_path) + if not m: + return {} + + # Map captured groups back to parameter names in order + params: dict[str, str] = {} + for name, value in zip(param_names, m.groups(), strict=False): + # Remove the leading slash for {/var} captures + params[name] = value.lstrip("/") + + return paramssrc/draive/parameters/specification.py (1)
446-456: Do not instantiate TypedDict classes at runtimeTypedDict types aren’t callable; returning ParameterAlternativesSpecification(...) will raise. Return plain dicts instead.
- return ParameterAlternativesSpecification( - type=compressed_alternatives, - description=description, - ) + return { + "type": compressed_alternatives, + "description": description, + } @@ - return ParameterAlternativesSpecification( - type=compressed_alternatives, - ) + return { + "type": compressed_alternatives, + }Also applies to: 461-465
src/draive/parameters/model.py (3)
444-446: PEP 695 generics: avoid requiring typing.Generic in basesClasses using the new generic syntax don’t inherit from typing.Generic; this assert will fail for e.g., class XT. Check type_params instead.
- assert Generic in cls.__bases__, "Can't specialize non generic type!" # nosec: B101 + assert getattr(cls, "__type_params__", None) is not None, "Can't specialize non-generic type!" # nosec: B101
633-683: vars(self) on slotted classes raises; adapt for slotsFor slotted DataModel subclasses without dict, vars(self) raises TypeError. Use FIELDS for membership, iteration, and length.
def __contains__( self, element: Any, ) -> bool: - return element in vars(self) + if getattr(self, "__slots__", ()): + return isinstance(element, str) and any(element == f.name for f in self.__FIELDS__) + else: + return element in vars(self) @@ def __iter__(self) -> Iterator[str]: - yield from vars(self) + if getattr(self, "__slots__", ()): + yield from (f.name for f in self.__FIELDS__) + else: + yield from vars(self) @@ def __len__(self) -> int: - return len(vars(self)) + if getattr(self, "__slots__", ()): + return len(self.__FIELDS__) + else: + return len(vars(self))
41-44: Add docstrings for Field and DataModelThese are public APIs; add concise NumPy-style docstrings.
As per coding guidelines
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
Makefile(1 hunks)pyproject.toml(2 hunks)src/draive/mcp/client.py(2 hunks)src/draive/openai/responses.py(7 hunks)src/draive/openai/utils.py(1 hunks)src/draive/parameters/__init__.py(0 hunks)src/draive/parameters/coding.py(1 hunks)src/draive/parameters/function.py(5 hunks)src/draive/parameters/model.py(18 hunks)src/draive/parameters/parameter.py(2 hunks)src/draive/parameters/specification.py(13 hunks)src/draive/parameters/validation.py(3 hunks)src/draive/resources/template.py(3 hunks)src/draive/vllm/messages.py(8 hunks)src/draive/vllm/utils.py(1 hunks)tests/test_model.py(2 hunks)tests/test_specification.py(2 hunks)tests/test_tags_extraction.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/draive/parameters/init.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use absolute imports from the draive package (e.g., from draive import ...), not relative imports
Follow Ruff import ordering: standard library, third-party, then local imports
Use Python 3.12+ typing features (e.g., | unions, PEP 695 generics)
Prefer abstract/base collection types (Sequence, Mapping, Set, Iterable) over concrete list, dict, set in type hints
Define and raise custom exception types for specific error cases
Use NumPy docstring convention for all functions, classes, and methods
Skip module-level docstrings unless explicitly requested
Docstrings should include Parameters, Returns, Raises, and Notes (if needed) sectionsFormat all Python code with Ruff formatter (run via
make format); do not use any other formatter
Files:
src/draive/openai/utils.pytests/test_model.pytests/test_tags_extraction.pysrc/draive/vllm/utils.pysrc/draive/vllm/messages.pysrc/draive/resources/template.pytests/test_specification.pysrc/draive/parameters/specification.pysrc/draive/parameters/validation.pysrc/draive/parameters/coding.pysrc/draive/openai/responses.pysrc/draive/mcp/client.pysrc/draive/parameters/function.pysrc/draive/parameters/parameter.pysrc/draive/parameters/model.py
src/draive/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/draive/**/*.py: Import Haiway symbols directly asfrom haiway import State, ctx
Usectx.scope(...)to bind activeStateinstances; avoid global state
Route all logging throughctx.log_*and never use
Enforce strict typing on public APIs; avoid untyped definitions andAnyexcept at verified third‑party boundaries
Prefer explicit attribute access with static types; avoid dynamicgetattrexcept at narrow boundaries
Use abstract immutable protocols (Mapping,Sequence,Iterable) instead of concretedict/list/setin public types
Usefinalwhere applicable; prefer composition over complex inheritance
Use precise unions (|), narrow withmatch/isinstance, and avoidcastunless provably safe and localized
Model immutable configuration/data ashaiway.State; avoid in‑place mutation and preferState.updated(...)/functional builders
Access active state viahaiway.ctxwithin async scopes (ctx.scope(...))
Public state methods that dispatch on the active instance must use@statemethod
Add structured, concise logs around generation calls, tool dispatch, and provider requests/responses (without secrets); add metrics viactx.recordwhere applicable
All I/O is async; keep boundaries async, usectx.spawnfor detached tasks, and avoid custom threading
Translate provider/SDK errors into typed exceptions; never raise bareException; preserve context
Compose multimodal content withMultimodalContent.of(...); useResourceContent/Referencefor blobs; wrap custom data inArtifactContent(use hidden when needed)
Public symbols require NumPy-style docstrings with Parameters/Returns/Raises and rationale when non-obvious
Avoid docstrings for internal helpers; prefer self-explanatory names; skip module-level docstrings
Never log secrets or full request bodies containing keys/tokens
Read credentials from environment variables; resolve via helpers likegetenv_str
Files:
src/draive/openai/utils.pysrc/draive/vllm/utils.pysrc/draive/vllm/messages.pysrc/draive/resources/template.pysrc/draive/parameters/specification.pysrc/draive/parameters/validation.pysrc/draive/parameters/coding.pysrc/draive/openai/responses.pysrc/draive/mcp/client.pysrc/draive/parameters/function.pysrc/draive/parameters/parameter.pysrc/draive/parameters/model.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Place tests in the tests/ directory and use pytest (with async support)
Mark asynchronous tests with pytest.mark.asyncio
Mock dependencies within test scope using stubbed functionality state (e.g., via ctx.scope)
tests/**/*.py: Unit tests must not perform network I/O; mock providers/HTTP
Usepytest-asyncio(@pytest.mark.asyncio) for coroutine tests
Prefer scoping withctx.scope(...)in tests and bind requiredStateinstances explicitly
Avoid real I/O in tests; stub provider calls and HTTP
Files:
tests/test_model.pytests/test_tags_extraction.pytests/test_specification.py
🧠 Learnings (1)
📚 Learning: 2025-09-11T15:22:15.967Z
Learnt from: CR
PR: miquido/draive#0
File: AGENTS.md:0-0
Timestamp: 2025-09-11T15:22:15.967Z
Learning: Applies to src/draive/__init__.py : Centralize and update all public exports in `src/draive/__init__.py` when API surface changes
Applied to files:
src/draive/parameters/specification.py
🧬 Code graph analysis (11)
src/draive/openai/utils.py (2)
src/draive/vllm/utils.py (1)
unwrap_missing(9-17)src/draive/gemini/utils.py (5)
unwrap_missing(10-14)unwrap_missing(18-22)unwrap_missing(26-32)unwrap_missing(36-42)unwrap_missing(45-59)
tests/test_tags_extraction.py (1)
src/draive/multimodal/content.py (4)
tags(427-446)MultimodalContent(25-592)of(42-66)of(619-647)
src/draive/vllm/messages.py (2)
src/draive/openai/utils.py (1)
unwrap_missing(9-17)src/draive/vllm/utils.py (1)
unwrap_missing(9-17)
src/draive/resources/template.py (2)
src/draive/parameters/parameter.py (2)
Parameter(14-143)find(80-98)src/draive/parameters/function.py (1)
ParametrizedFunction(136-271)
tests/test_specification.py (1)
src/draive/parameters/specification.py (1)
parameter_specification(204-229)
src/draive/parameters/validation.py (2)
src/draive/parameters/parameter.py (2)
of(16-55)validate(127-143)src/draive/parameters/model.py (3)
validate(323-326)validate(521-533)to_mapping(546-554)
src/draive/openai/responses.py (2)
src/draive/openai/utils.py (1)
unwrap_missing(9-17)src/draive/vllm/utils.py (1)
unwrap_missing(9-17)
src/draive/mcp/client.py (2)
src/draive/models/tools/types.py (1)
parameters(79-79)src/draive/parameters/specification.py (1)
ToolParametersSpecification(190-201)
src/draive/parameters/function.py (1)
src/draive/parameters/parameter.py (3)
validate(127-143)find(80-98)pick(100-125)
src/draive/parameters/parameter.py (1)
src/draive/parameters/model.py (2)
validate(323-326)validate(521-533)
src/draive/parameters/model.py (3)
src/draive/parameters/coding.py (2)
ParametersJSONEncoder(11-26)default(12-26)src/draive/parameters/parameter.py (4)
Parameter(14-143)of(16-55)validate(127-143)find(80-98)src/draive/parameters/validation.py (3)
of(36-65)validator(57-60)validator(94-105)
🔇 Additional comments (25)
pyproject.toml (1)
8-24: Version bump to match dependency upgrade is soundThe project version and haiway constraint move forward together, which keeps packaging consistent with the dependency upgrade. No issues spotted.
Makefile (1)
13-13: UV toolchain update looks goodBumping the pinned UV version to 0.8.22 keeps the bootstrap script current and the version gate still works with the existing comparison logic.
src/draive/vllm/utils.py (1)
4-4: LGTM: sentinel migration to Omit/omit is correct.Import and usage align with the updated OpenAI SDK semantics.
src/draive/parameters/function.py (1)
227-243: Rename to Parameter.validate: good migration.Swapping validated→validate matches the new Parameter API and keeps error scoping under ValidationContext.
src/draive/vllm/messages.py (4)
171-187: Response format: omit semantics look correct.Type hint and defaulting to omit align with SDK expectations; json/json_schema branches preserved.
204-206: Conditional omission for optional params is correct.Using omit for parallel_tool_calls and stop when absent prevents sending unwanted keys.
Also applies to: 211-213, 356-358, 363-364
625-633: Return (omit, omit) when no tools.Correctly omits both tool_choice and tools for empty tool sets; signatures updated accordingly.
8-8: Confirmed:pyproject.tomlpinsopenai~=1.104, which includesOmit/omitsupport.src/draive/parameters/validation.py (2)
43-66: Validator composition is sound.Wrapping base validator for object-attribute handling and layering verifier preserves error locality and typing.
83-107: Smart coercion for DataModel generics.The origin/parameters check with to_mapping fallback is a pragmatic solution to reconcile mismatched generic parameters.
Also applies to: 110-121, 137-158
tests/test_tags_extraction.py (1)
78-79: LGTM: formatting-only simplification.Single-line construction preserves behavior.
src/draive/parameters/coding.py (1)
11-26: Encoder logic is correct and safe.Order of isinstance checks avoids datetime-as-date pitfalls; UUID/datetime/date/time encodings are appropriate. LGTM.
tests/test_model.py (2)
521-528: Streaming iterator path is clear and deterministic.Yielding text, then image, then audio simplifies expectations for the selection layer. LGTM.
543-543: Concise async collection.The single-line async comprehension is fine and equivalent to the prior multi-step approach.
src/draive/openai/responses.py (3)
463-491: Mirror check for stream path unwrap_missing + omit usage.Same potential signature issue as above in the streaming call. If unwrap_missing doesn’t accept default, remove the argument and rely on its built-in omit behavior.
Based on learnings
Use the same script as above to verify.
658-666: Text output omission when output='auto' is correct.Returning omit here aligns with the SDK’s sentinel expectations and avoids sending redundant defaults.
217-246: Verify haiway.unwrap_missing supports default parameter
unwrap_missing(config.temperature, default=omit) may not accept adefaultargument—confirm its signature in the haiway package and adjust accordingly.src/draive/parameters/parameter.py (2)
4-4: Imports align with the updated validation API.Switch to AttributeAnnotation and Validator is consistent with the broader refactor.
127-144: Rename to validate and streamlined logic looks good.Validation scoping and default handling are sound; the API now matches ParametrizedFunction.validate_arguments.
src/draive/resources/template.py (2)
351-367: Coercion pipeline for URI params is solid.Mapping raw URI strings into typed kwargs using specification-driven coercion is a good approach and remains a no-op for unknown keys. LGTM.
525-540: Happy path for resolve_from_uri reads cleanly.Pre-coercion + validated call + proper error wrapping into ResourceCorrupted is correct.
src/draive/mcp/client.py (1)
31-31: Confirm public export path for DataModelImport switched to draive.parameters. Ensure DataModel remains re-exported from draive/init.py to keep the stable top-level API, or import from draive for consistency.
Based on learnings
tests/test_specification.py (1)
21-101: LGTM: attribute-based specifications and compressed unionsThe move to dedicated Attribute types and compressed union types is coherent and matches the new specification dispatch.
src/draive/parameters/specification.py (1)
536-573: Enum handling looks goodString and integer enum specifications correctly populate enum arrays and optional descriptions.
src/draive/openai/utils.py (1)
4-4: Add docstring and confirm SDK supports Omit/omitApply this NumPy-style docstring diff to
unwrap_missing(lines 12–15):def unwrap_missing[Value]( value: Value | Missing, /, ) -> Value | Omit: + """Return omit for MISSING; otherwise return the original value. + + Parameters + ---------- + value : Value | Missing + Value to unwrap. + + Returns + ------- + Value | Omit + omit when value is MISSING; otherwise the original value. + """ if value is MISSING: return omitConfirm that the pinned
openai~=1.104exposesOmitandomit; bump the version if it doesn’t.
| parameters=cast( | ||
| ToolParametersSpecification, | ||
| { | ||
| **mcp_tool.inputSchema, | ||
| "additionalProperties": False, | ||
| } | ||
| }, | ||
| ), |
There was a problem hiding this comment.
Validate/normalize MCP tool inputSchema instead of blind cast
mcp_tool.inputSchema can be None or a non-object schema. Spreading it and casting will raise at runtime and/or violate ToolParametersSpecification invariants (type must be "object"). Build a safe, strict object schema and propagate optional fields.
Apply this diff in-place:
- parameters=cast(
- ToolParametersSpecification,
- {
- **mcp_tool.inputSchema,
- "additionalProperties": False,
- },
- ),
+ parameters=_coerce_tool_parameters_spec(mcp_tool.inputSchema),Add this helper (outside the shown range, e.g., below _convert_tool or near module top):
def _coerce_tool_parameters_spec(
input_schema: Mapping[str, Any] | None,
) -> ToolParametersSpecification:
if not isinstance(input_schema, Mapping):
ctx.log_warning("Missing or invalid MCP tool inputSchema; defaulting to strict object")
return {
"type": "object",
"properties": {},
"additionalProperties": False,
}
type_value = input_schema.get("type", "object")
if type_value != "object":
ctx.log_warning(f"Unexpected MCP tool inputSchema.type={type_value!r}; forcing 'object'")
spec: ToolParametersSpecification = {
"type": "object",
"properties": cast(Mapping[str, Any], input_schema.get("properties", {})),
"additionalProperties": False,
}
required = input_schema.get("required")
if isinstance(required, Sequence):
spec["required"] = [str(k) for k in required] # type: ignore[assignment]
title = input_schema.get("title")
if isinstance(title, str):
spec["title"] = title # type: ignore[assignment]
description = input_schema.get("description")
if isinstance(description, str):
spec["description"] = description # type: ignore[assignment]
return spec🤖 Prompt for AI Agents
In src/draive/mcp/client.py around lines 483 to 489, the code blindly casts
mcp_tool.inputSchema into ToolParametersSpecification which can be None or a
non-object schema; create a helper _coerce_tool_parameters_spec(input_schema) as
described and use it instead of the cast so you always produce a strict object
schema (type "object", properties from input if mapping, additionalProperties
False) while preserving optional fields title, description, and required (coerce
required to strings if sequence), and emit ctx.log_warning when input_schema is
missing/invalid or its type is not "object"; place the helper near the module
top or below _convert_tool and replace the current parameters=cast(...) call
with parameters=_coerce_tool_parameters_spec(mcp_tool.inputSchema).
| def parameter_specification( | ||
| annotation: AttributeAnnotation, | ||
| /, | ||
| description: str | None, | ||
| ) -> ParameterSpecification: | ||
| if specification := SPECIFICATIONS.get(annotation.origin): | ||
| if isinstance(annotation, AliasAttribute): | ||
| return parameter_specification(annotation.resolved, description) | ||
|
|
||
| if specification := SPECIFICATIONS.get(type(annotation)): | ||
| return specification(annotation, description) | ||
|
|
||
| elif hasattr(annotation.origin, "__PARAMETERS_SPECIFICATION__"): | ||
| elif hasattr(annotation.base, "__PARAMETERS_SPECIFICATION__"): | ||
| if description := description: | ||
| return cast( | ||
| ParameterSpecification, | ||
| { | ||
| **annotation.origin.__PARAMETERS_SPECIFICATION__, | ||
| **annotation.base.__PARAMETERS_SPECIFICATION__, | ||
| "description": description, | ||
| }, | ||
| ) | ||
|
|
||
| else: | ||
| return annotation.origin.__PARAMETERS_SPECIFICATION__ | ||
|
|
||
| elif is_typeddict(annotation.origin): | ||
| return _prepare_specification_of_typed_dict( | ||
| annotation, | ||
| description=description, | ||
| ) | ||
|
|
||
| elif issubclass(annotation.origin, IntEnum): | ||
| if description := description: | ||
| return { | ||
| "type": "integer", | ||
| "enum": list(annotation.origin), | ||
| "description": description, | ||
| } | ||
|
|
||
| else: | ||
| return { | ||
| "type": "integer", | ||
| "enum": list(annotation.origin), | ||
| } | ||
|
|
||
| elif issubclass(annotation.origin, StrEnum): | ||
| if description := description: | ||
| return { | ||
| "type": "string", | ||
| "enum": list(annotation.origin), | ||
| "description": description, | ||
| } | ||
|
|
||
| else: | ||
| return { | ||
| "type": "string", | ||
| "enum": list(annotation.origin), | ||
| } | ||
| return annotation.base.__PARAMETERS_SPECIFICATION__ | ||
|
|
||
| else: | ||
| raise TypeError(f"Unsupported type annotation: {annotation}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add docstring to parameter_specification
Public API should have NumPy-style docstrings per guidelines.
As per coding guidelines
🤖 Prompt for AI Agents
In src/draive/parameters/specification.py around lines 204 to 229, add a
NumPy-style docstring to the public function parameter_specification; include a
short summary line, parameter entries for "annotation" (type
AttributeAnnotation) and "description" (str | None), a "Returns" section
describing the returned ParameterSpecification, a "Raises" section documenting
the TypeError for unsupported annotations, and any pertinent notes about alias
resolution or lookup behavior (e.g., that AliasAttribute is resolved and
SPECIFICATIONS and __PARAMETERS_SPECIFICATION__ are consulted); ensure
formatting follows the project's NumPy-style docstring conventions.
| return cast( | ||
| Validator[Dict], | ||
| cls.of( | ||
| resolve_attribute( | ||
| typed_dict, | ||
| module=typed_dict.__module__, | ||
| resolved_parameters={}, # TODO: type parameters? | ||
| recursion_guard={}, | ||
| ), | ||
| verifier=None, | ||
| ), | ||
| verifier=None, | ||
| recursion_guard={}, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
TypedDict path uses resolve_attribute correctly; track TODO.
The recursion inputs look fine; the TODO about type parameters is noted.
I can help add propagation of resolved type parameters if you provide expected shapes/usages. Should I open a follow-up issue to track this?
🤖 Prompt for AI Agents
In src/draive/parameters/validation.py around lines 22 to 33, update the
resolve_attribute call so it forwards the current resolved type parameters
instead of passing an empty dict for resolved_parameters; identify where the
type parameter mappings are tracked in the calling context (or the
Validator/Class that invoked this) and pass that mapping through, preserve and
propagate recursion_guard as-is, and ensure subsequent recursive calls also
accept and forward the resolved_parameters mapping; if the expected shape/source
of the type-parameter mapping is unclear, create a follow-up issue documenting
the required mapping shape and where it should originate so the TODO can be
implemented consistently.
| def _coerce_from_specification( # noqa: C901, PLR0911, PLR0912 | ||
| self, | ||
| specification: ParameterSpecification, | ||
| value: str, | ||
| ) -> tuple[Any, bool]: | ||
| if isinstance(specification, Mapping) and "oneOf" in specification: | ||
| for option in specification["oneOf"]: | ||
| converted, changed = self._coerce_from_specification(option, value) | ||
| if changed: | ||
| return converted, True | ||
|
|
||
| return value, False | ||
|
|
||
| if isinstance(specification, Mapping): | ||
| type_name: Any | None = specification.get("type") | ||
| if type_name == "integer": | ||
| try: | ||
| return int(value), True | ||
| except ValueError: | ||
| return value, False | ||
|
|
||
| if type_name == "number": | ||
| try: | ||
| return float(value), True | ||
| except ValueError: | ||
| return value, False | ||
|
|
||
| if type_name == "boolean": | ||
| parsed_bool = self._parse_boolean(value) | ||
| if parsed_bool is not None: | ||
| return parsed_bool, True | ||
|
|
||
| return value, False | ||
|
|
||
| if type_name == "array": | ||
| parsed_array = self._parse_json_sequence(value) | ||
| if parsed_array is not None: | ||
| return parsed_array, True | ||
|
|
||
| return value, False | ||
|
|
||
| if type_name == "object": | ||
| parsed_object = self._parse_json_mapping(value) | ||
| if parsed_object is not None: | ||
| return parsed_object, True | ||
|
|
||
| return value, False | ||
|
|
||
| if type_name == "null": | ||
| normalised = value.strip().lower() | ||
| if normalised in {"null", "none"}: | ||
| return None, True | ||
|
|
||
| return value, False | ||
|
|
||
| if "enum" in specification: | ||
| converted_enum, enum_changed = self._match_enum(specification["enum"], value) | ||
| if enum_changed: | ||
| return converted_enum, True | ||
|
|
||
| return value, False | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: Normalize specification to a mapping to harden coercion.
If ParameterSpecification isn’t always a Mapping, consider normalizing once with as_dict(specification) to ensure consistent key access across providers/versions.
As per coding guidelines
🤖 Prompt for AI Agents
In src/draive/resources/template.py around lines 379 to 440, the method
_coerce_from_specification assumes ParameterSpecification is a Mapping;
normalize the incoming specification once at the top by converting it to a dict
(e.g. spec = as_dict(specification) or similar helper) and then use spec for all
key lookups (spec.get("oneOf"), spec.get("type"), "enum" in spec, etc.),
replacing the repeated isinstance(...) checks so the function consistently
accesses keys from the normalized mapping and remains robust across
provider/version variations.
| def unwrap_missing[Value]( | ||
| value: Value | Missing, | ||
| /, | ||
| ) -> Value | NotGiven: | ||
| ) -> Value | Omit: | ||
| if value is MISSING: | ||
| return NOT_GIVEN | ||
| return omit | ||
|
|
||
| else: | ||
| return cast(Value, value) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add a NumPy-style docstring for public API.
unwrap_missing is exported via all; per guidelines, public functions require NumPy-style docstrings.
As per coding guidelines
def unwrap_missing[Value](
value: Value | Missing,
/,
) -> Value | Omit:
+ """
+ Unwrap haiway.MISSING into the OpenAI omit sentinel.
+
+ Parameters
+ ----------
+ value : Value | Missing
+ Input value or the MISSING sentinel.
+
+ Returns
+ -------
+ Value | Omit
+ The input value when present; otherwise the omit sentinel.
+ """
if value is MISSING:
return omit📝 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.
| def unwrap_missing[Value]( | |
| value: Value | Missing, | |
| /, | |
| ) -> Value | NotGiven: | |
| ) -> Value | Omit: | |
| if value is MISSING: | |
| return NOT_GIVEN | |
| return omit | |
| else: | |
| return cast(Value, value) | |
| def unwrap_missing[Value]( | |
| value: Value | Missing, | |
| /, | |
| ) -> Value | Omit: | |
| """ | |
| Unwrap haiway.MISSING into the OpenAI omit sentinel. | |
| Parameters | |
| ---------- | |
| value : Value | Missing | |
| Input value or the MISSING sentinel. | |
| Returns | |
| ------- | |
| Value | Omit | |
| The input value when present; otherwise the omit sentinel. | |
| """ | |
| if value is MISSING: | |
| return omit | |
| else: | |
| return cast(Value, value) |
🤖 Prompt for AI Agents
In src/draive/vllm/utils.py around lines 9 to 17, the public function
unwrap_missing (exported via __all__) lacks a NumPy-style docstring; add a clear
NumPy-style docstring immediately above the function that briefly describes what
unwrap_missing does, documents the parameters (value: Value | Missing) and their
meaning, the return value (Value | Omit) and when omit is returned, and any side
effects or notes (e.g., that MISSING yields omit); keep wording concise and
include types under Parameters and Returns sections to satisfy the public API
guidelines.
| SequenceAttribute( | ||
| base=Sequence, | ||
| values=StringAttribute(), | ||
| ), | ||
| description=None, | ||
| ) == {"type": "array", "items": {"type": "string"}} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove duplicated sequence spec assertion
This block duplicates the earlier SequenceAttribute(string) assertion; keep one to reduce noise.
- assert parameter_specification(
- SequenceAttribute(
- base=Sequence,
- values=StringAttribute(),
- ),
- description=None,
- ) == {"type": "array", "items": {"type": "string"}}📝 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.
| SequenceAttribute( | |
| base=Sequence, | |
| values=StringAttribute(), | |
| ), | |
| description=None, | |
| ) == {"type": "array", "items": {"type": "string"}} |
🤖 Prompt for AI Agents
In tests/test_specification.py around lines 77 to 82, there is a duplicated
assertion checking SequenceAttribute(base=Sequence, values=StringAttribute(),
description=None) == {"type": "array", "items": {"type": "string"}}; remove this
duplicate assertion so only the original SequenceAttribute(string) assertion
remains to avoid redundant tests and noise.
KaQuMiQ
force-pushed
the
feature/haiway_attributes
branch
from
fc317ec to
ae6984f
Compare
1 participant
No description provided.