Conversation
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (22)
.github/workflows/ci.yml(1 hunks)CLAUDE.md(0 hunks)Makefile(3 hunks)docs/cookbooks/BasicDataExtraction.md(4 hunks)docs/cookbooks/BasicMCP.md(0 hunks)docs/cookbooks/BasicRAG.md(0 hunks)docs/getting-started/first-steps.md(1 hunks)docs/getting-started/installation.md(1 hunks)docs/getting-started/quickstart.md(1 hunks)docs/guides/AdvancedState.md(0 hunks)docs/guides/BasicConversation.md(0 hunks)docs/guides/BasicModelGeneration.md(0 hunks)docs/guides/BasicToolsUse.md(0 hunks)docs/guides/BasicUsage.md(0 hunks)docs/guides/Basics.md(0 hunks)docs/guides/ComprehensiveEvaluation.md(0 hunks)docs/guides/EvaluatorCatalog.md(12 hunks)docs/index.md(0 hunks)pyproject.toml(1 hunks)src/draive/ollama/chat.py(2 hunks)tests/ollama/test_schema.py(3 hunks)tools/markdown_lint.py(1 hunks)
💤 Files with no reviewable changes (11)
- CLAUDE.md
- docs/guides/ComprehensiveEvaluation.md
- docs/guides/BasicUsage.md
- docs/guides/BasicModelGeneration.md
- docs/index.md
- docs/guides/Basics.md
- docs/guides/AdvancedState.md
- docs/guides/BasicConversation.md
- docs/guides/BasicToolsUse.md
- docs/cookbooks/BasicMCP.md
- docs/cookbooks/BasicRAG.md
🧰 Additional context used
📓 Path-based instructions (8)
docs/**
📄 CodeRabbit inference engine (CLAUDE.md)
docs/**: If behavior/API changes, update relevant docs under docs/ and examples as needed
When adding public APIs, update examples/guides and ensure cross-links render
Update documentation under docs/ when behavior/API changes
Files:
docs/guides/EvaluatorCatalog.mddocs/getting-started/installation.mddocs/getting-started/quickstart.mddocs/cookbooks/BasicDataExtraction.mddocs/getting-started/first-steps.md
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
When behavior/API changes, update relevant documentation pages under
docs/and examples as needed
Files:
docs/guides/EvaluatorCatalog.mddocs/getting-started/installation.mddocs/getting-started/quickstart.mddocs/cookbooks/BasicDataExtraction.mddocs/getting-started/first-steps.md
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Unit tests must not perform network I/O; mock providers/HTTP
Keep tests fast and specific to changed code; start with unit tests for new types/functions/adapters
Use fixtures from tests/ or add focused ones; avoid heavy integration scaffolding
Use pytest-asyncio for coroutine tests (@pytest.mark.asyncio)
Prefer scoping with ctx.scope(...) and bind required State instances explicitly in async tests
Avoid real I/O and network in tests; stub provider calls and HTTP
tests/**/*.py: Do not perform network I/O in unit tests; mock providers and HTTP
Keep tests fast and focused; prefer targeted unit tests and lightweight fixtures over heavy integration scaffolding
Usepytest-asynciofor coroutine tests (@pytest.mark.asyncio)
Scope tests withctx.scope(...)and bind requiredStateinstances explicitly
Avoid real I/O in async tests; stub provider calls and HTTP
Files:
tests/ollama/test_schema.py
{src/draive,tests}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Target Python 3.12+ features and typing syntax
Files:
tests/ollama/test_schema.pysrc/draive/ollama/chat.py
src/draive/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/draive/**/*.py: Import Haiway symbols directly, e.g., from haiway import State, ctx
Use ctx.scope(...) for context scoping and to avoid global state
All logs must use ctx.log_*; do not use print
Use strict typing with Python 3.12+ syntax; no untyped public APIs; avoid Any except at third‑party boundaries
Prefer explicit attribute access with static types; avoid dynamic getattr except at narrow boundaries
Prefer immutable protocols (Mapping, Sequence, Iterable) in public types over dict/list/set
Use final where applicable; avoid complex inheritance, prefer composition
Use precise unions (|) and narrow with match/isinstance; avoid cast unless provably safe and localized
Represent data/config and service facades as haiway.State; provide ergonomic constructors like .of(...)
Avoid in-place mutation of State; use State.updated(...) or functional builders
Access active state through haiway.ctx inside async scopes (ctx.scope(...))
Public state methods that dispatch on the active instance should use @statemethod
Record metrics using ctx.record where applicable
Prefer structured and concise log messages; avoid verbosity in hot paths
All I/O is async; keep boundaries async and use ctx.spawn for detached tasks
Rely on haiway and asyncio; avoid custom threading
Do not raise bare Exception; include contextual information in exceptions
Public symbols should have NumPy-style docstrings with Parameters/Returns/Raises
Avoid docstrings for internal helpers; keep names self-explanatory
Keep docstrings high-quality; mkdocstrings pulls them into reference pages
src/draive/**/*.py: Import Haiway symbols directly:from haiway import State, ctx
Usectx.scope(...)to bind scoped Disposables and active State; avoid global state
Route all logs throughctx.log_*; do not use
No untyped public APIs; maintain strict typing
Avoid looseAnyexcept at explicit third‑party boundaries
PreferMapping/Sequence/Iterableover concretedict/list/setin public types
Use `...
Files:
src/draive/ollama/chat.py
src/draive/{generation,models,openai,anthropic,mistral,gemini,vllm,ollama,bedrock,cohere}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Log around generation calls, tool dispatch, and provider requests/responses (without leaking secrets)
Files:
src/draive/ollama/chat.py
src/draive/{openai,anthropic,mistral,gemini,vllm,ollama,bedrock,cohere}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Translate provider/SDK errors into typed exceptions
Files:
src/draive/ollama/chat.py
src/draive/{openai,anthropic,mistral,gemini,vllm,ollama,bedrock,cohere,httpx}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Never log secrets or full request bodies containing keys/tokens
Files:
src/draive/ollama/chat.py
🧠 Learnings (1)
📚 Learning: 2025-10-01T13:12:39.671Z
Learnt from: CR
PR: miquido/draive#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T13:12:39.671Z
Learning: Run linters/type-checkers Ruff, Bandit, Pyright (strict) via `make lint`
Applied to files:
Makefile.github/workflows/ci.yml
🧬 Code graph analysis (1)
tests/ollama/test_schema.py (2)
src/draive/parameters/model.py (1)
DataModel(418-768)src/draive/ollama/chat.py (2)
_schema_for_ollama(392-402)_response_format(583-601)
🪛 checkmake (0.2.2)
Makefile
[warning] 16-16: Missing required phony target "all"
(minphony)
[warning] 16-16: Missing required phony target "clean"
(minphony)
🪛 markdownlint-cli2 (0.18.1)
docs/guides/EvaluatorCatalog.md
77-77: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (macos-latest, 3.12)
🔇 Additional comments (19)
pyproject.toml (1)
8-8: LGTM!Version bump to 0.87.5 is appropriate for this documentation and tooling update.
Makefile (4)
16-16: LGTM!The
.PHONYdeclaration correctly includes the newdocs-linttarget.
80-80: LGTM!Integrating Markdown linting into the standard
linttarget ensures documentation quality is checked alongside code quality. This aligns with the retrieved learning thatmake lintruns all linters and type-checkers.Based on learnings.
82-83: LGTM!The standalone
docs-linttarget provides a focused way to validate documentation without running the full lint suite.
97-97: LGTM!Adding
--strictto the MkDocs build ensures that warnings are treated as errors, preventing broken cross-references and other issues from slipping into production documentation.docs/cookbooks/BasicDataExtraction.md (1)
39-39: LGTM!Adding the optional
countryfield to thePersonalDatamodel enriches the example and demonstrates nullable field handling. All downstream examples are updated consistently to reflect this change.docs/getting-started/installation.md (1)
1-52: LGTM!The rewritten installation documentation provides clear, step-by-step instructions for both
pipanduvworkflows. The addition of provider extras examples, optional dependencies, and a verification step significantly improves the user onboarding experience.tools/markdown_lint.py (6)
1-12: LGTM!Imports and regex patterns are well-structured. The code fence pattern correctly handles variable-length backtick delimiters, and the heading pattern properly captures heading levels.
14-23: LGTM!The
LintErrordataclass provides a clean error representation with a standard linter output format.
25-31: LGTM!The
iter_markdown_filesfunction correctly handles both individual files and recursive directory traversal, with sorted output for deterministic linting results.
33-99: LGTM!The
lint_markdownfunction implements comprehensive Markdown quality checks with robust code fence handling. The fence stack correctly handles nested code fences, and the heading progression logic prevents confusing document structure. The check for H1 as the first content line ensures proper document hierarchy.
102-127: LGTM!The
mainfunction implements a standard CLI pattern with appropriate error handling. Returning non-zero when no Markdown files are found prevents silent failures.
129-130: LGTM!The entry point uses the preferred
raise SystemExit(main())pattern for clean exit code handling..github/workflows/ci.yml (1)
34-35: LGTM!The new "Docs Lint" step is well-positioned in the CI pipeline, ensuring documentation quality is validated after code linting but before running tests. The use of
uv runensures the linter runs in the correct Python environment.docs/getting-started/quickstart.md (1)
1-141: LGTM!The rewritten quickstart guide provides an excellent progression from basic generation through tools, structured data extraction, and multi-provider usage. The addition of prerequisites, environment setup, and complete runnable examples significantly improves the onboarding experience.
tests/ollama/test_schema.py (4)
24-26: LGTM: focused fixtures for nullable and unsupported unionsFixtures are clear and targeted.
Also applies to: 28-30
92-103: Confirm intent: nullable scalar is collapsed to non-null and stays requiredThis asserts required still includes the field after dropping nullability. Please confirm this is the intended Ollama contract; otherwise we should also drop it from "required".
105-107: LGTM: unsupported unions correctly rejectedBehavior aligns with provider fallback expectations.
117-117: LGTM: response format falls back to JSON with warningCovers the fallback path and observability.
| from draive import TextGeneration | ||
|
|
||
| async def setup_rag_system(document: str): | ||
| async def tagline() -> str: | ||
| async with ctx.scope( | ||
| "rag", | ||
| # Prepare in-memory vector index | ||
| VolatileVectorIndex(), | ||
| OpenAIEmbeddingConfig(model="text-embedding-3-small"), | ||
| "tagline", | ||
| OpenAIResponsesConfig(model="gpt-4o-mini"), | ||
| disposables=(OpenAI(),), | ||
| ): | ||
| # Split document into chunks | ||
| document_chunks = [ | ||
| DocumentChunk( | ||
| full_document=document, | ||
| content=chunk, | ||
| ) | ||
| for chunk in split_text( | ||
| text=document, | ||
| separators=("\n\n", " "), | ||
| part_size=64, | ||
| part_overlap_size=16, | ||
| count_size=len, | ||
| ) | ||
| ] | ||
|
|
||
| # Index the chunks | ||
| await VectorIndex.index( | ||
| DocumentChunk, | ||
| values=document_chunks, | ||
| attribute=DocumentChunk._.content, # Use AttributePath | ||
| return await TextGeneration.generate( | ||
| instructions="You are a branding assistant", | ||
| input="Create a one-line pitch for a travel planner", | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make the tagline snippet self-contained.
The snippet uses ctx, OpenAIResponsesConfig, and OpenAI but only imports TextGeneration. Adding the remaining imports here keeps the example copy-pasteable without readers having to reconcile earlier sections.
🤖 Prompt for AI Agents
In docs/getting-started/first-steps.md around lines 57 to 68, the tagline
snippet only imports TextGeneration but references ctx, OpenAIResponsesConfig,
and OpenAI; make the example self-contained by adding the missing imports (e.g.,
import ctx, OpenAIResponsesConfig, and OpenAI from the draive package at the top
of the snippet) so readers can copy-paste and run the snippet without needing
earlier sections; ensure the import list includes all symbols used in the
snippet and keep the rest of the code unchanged.
| **Best for**: All content types as a basic quality check, especially user-facing text. | ||
|
|
||
| ### Readability Evaluator | ||
| ### Readability Evaluator |
There was a problem hiding this comment.
Add blank line before heading.
The heading at line 77 lacks the required blank line above it, as flagged by the Markdown linter.
Apply this diff to fix the spacing:
+
### Readability Evaluator📝 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.
| ### Readability Evaluator | |
| ### Readability Evaluator |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
77-77: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
In docs/guides/EvaluatorCatalog.md around line 77, the Markdown heading "###
Readability Evaluator" is missing a blank line above it; insert a single empty
line immediately before that heading to satisfy the linter and preserve proper
markdown heading separation.
| elements: list[Any] = list(schema_type) | ||
| collapsed: str | None = _collapse_ollama_type_union(elements) | ||
| if collapsed is None: | ||
| return None, changed | ||
|
|
||
| if len(elements) != 1 or elements[0] != collapsed: | ||
| changed = True | ||
|
|
||
| schema_type = collapsed | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add debug log when union collapse is unsupported
Helps trace why schemas fall back to generic JSON later.
- collapsed: str | None = _collapse_ollama_type_union(elements)
- if collapsed is None:
- return None, changed
+ collapsed: str | None = _collapse_ollama_type_union(elements)
+ if collapsed is None:
+ ctx.log_debug(f"ollama schema union unsupported: {elements}")
+ return None, changed📝 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.
| elements: list[Any] = list(schema_type) | |
| collapsed: str | None = _collapse_ollama_type_union(elements) | |
| if collapsed is None: | |
| return None, changed | |
| if len(elements) != 1 or elements[0] != collapsed: | |
| changed = True | |
| schema_type = collapsed | |
| elements: list[Any] = list(schema_type) | |
| - collapsed: str | None = _collapse_ollama_type_union(elements) | |
| - if collapsed is None: | |
| collapsed: str | None = _collapse_ollama_type_union(elements) | |
| if collapsed is None: | |
| ctx.log_debug(f"ollama schema union unsupported: {elements}") | |
| return None, changed | |
| if len(elements) != 1 or elements[0] != collapsed: | |
| changed = True | |
| schema_type = collapsed |
🤖 Prompt for AI Agents
In src/draive/ollama/chat.py around lines 450 to 459, when
_collapse_ollama_type_union returns None you should emit a debug log explaining
the unsupported union collapse so callers can trace why the schema falls back to
generic JSON; add a debug-level log in that branch that includes the original
schema_type (or the elements list) and any contextual info available (e.g.,
function name or identifier) so the message reads something like "Unsupported
union collapse for schema elements: [...] — falling back to JSON", using the
module logger (logging.getLogger(__name__) or the existing logger variable) and
do not change the current return/changed behavior.
| def test_schema_for_ollama_collapses_supported_unions(monkeypatch: pytest.MonkeyPatch) -> None: | ||
| logs: list[str] = [] | ||
|
|
||
| def fake_log_debug(message: str, *_, **__) -> None: | ||
| logs.append(message) | ||
|
|
||
| monkeypatch.setattr("draive.ollama.chat.ctx.log_debug", fake_log_debug, raising=False) | ||
|
|
||
| schema = _schema_for_ollama(_UnionSchema) | ||
|
|
||
| assert schema == { | ||
| "type": "object", | ||
| "properties": { | ||
| "value": {"type": "string"}, | ||
| }, | ||
| "required": ["value"], | ||
| "additionalProperties": False, | ||
| } | ||
| assert logs, "Expected debug log when schema normalization adjusts union types" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Solid union-collapse test; consider a few more cases
Add cases for:
- {"number","integer"} and {"number","integer","null"} → "number"
- {"object","null"} and {"array","null"} → collapsed to non-null
- Unsupported combos like {"string","boolean","null"} → None
Parametrize to reduce duplication.
🤖 Prompt for AI Agents
tests/ollama/test_schema.py around lines 71 to 90: the existing test validates
one union-collapse case but reviewer asks to add parameterized cases; convert
this single test into a pytest.mark.parametrize that supplies multiple input
union schemas and expected outputs: 1) {"number","integer"} → collapse to
"number"; 2) {"number","integer","null"} → collapse to "number" (non-null); 3)
{"object","null"} → collapsed to the non-null object schema; 4) {"array","null"}
→ collapsed to the non-null array schema; and 5) unsupported combos like
{"string","boolean","null"} → expect None. Keep the monkeypatching of
draive.ollama.chat.ctx.log_debug (or assert logs when normalization occurs) and
update assertions per case (expected schema or None and whether logs present),
so the single test covers all scenarios via parametrization to reduce
duplication and enable [request_verification].
KaQuMiQ
force-pushed
the
feaure/docs_lint
branch
from
771d1d2 to
0ce2a6a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/guides/EvaluatorCatalog.md (1)
77-77: Fix the missing blank line above heading.This heading still lacks the required blank line above it, as flagged by the Markdown linter and a previous review comment.
Apply this diff to fix the spacing:
+ ### Readability Evaluator
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (23)
.github/workflows/ci.yml(1 hunks)CLAUDE.md(0 hunks)Makefile(3 hunks)docs/cookbooks/BasicDataExtraction.md(4 hunks)docs/cookbooks/BasicMCP.md(0 hunks)docs/cookbooks/BasicRAG.md(0 hunks)docs/getting-started/first-steps.md(1 hunks)docs/getting-started/installation.md(1 hunks)docs/getting-started/quickstart.md(1 hunks)docs/guides/AdvancedState.md(0 hunks)docs/guides/BasicConversation.md(0 hunks)docs/guides/BasicModelGeneration.md(0 hunks)docs/guides/BasicToolsUse.md(0 hunks)docs/guides/BasicUsage.md(0 hunks)docs/guides/Basics.md(0 hunks)docs/guides/ComprehensiveEvaluation.md(0 hunks)docs/guides/EvaluatorCatalog.md(12 hunks)docs/index.md(0 hunks)llms.txt(1 hunks)pyproject.toml(1 hunks)src/draive/ollama/chat.py(3 hunks)tests/ollama/test_schema.py(3 hunks)tools/markdown_lint.py(1 hunks)
💤 Files with no reviewable changes (11)
- docs/guides/BasicModelGeneration.md
- docs/guides/AdvancedState.md
- docs/index.md
- docs/guides/BasicConversation.md
- docs/guides/ComprehensiveEvaluation.md
- docs/cookbooks/BasicRAG.md
- docs/guides/BasicUsage.md
- docs/guides/Basics.md
- docs/cookbooks/BasicMCP.md
- docs/guides/BasicToolsUse.md
- CLAUDE.md
🧰 Additional context used
📓 Path-based instructions (8)
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Unit tests must not perform network I/O; mock providers/HTTP
Keep tests fast and specific to changed code; start with unit tests for new types/functions/adapters
Use fixtures from tests/ or add focused ones; avoid heavy integration scaffolding
Use pytest-asyncio for coroutine tests (@pytest.mark.asyncio)
Prefer scoping with ctx.scope(...) and bind required State instances explicitly in async tests
Avoid real I/O and network in tests; stub provider calls and HTTP
tests/**/*.py: Do not perform network I/O in unit tests; mock providers and HTTP
Keep tests fast and focused; prefer targeted unit tests and lightweight fixtures over heavy integration scaffolding
Usepytest-asynciofor coroutine tests (@pytest.mark.asyncio)
Scope tests withctx.scope(...)and bind requiredStateinstances explicitly
Avoid real I/O in async tests; stub provider calls and HTTP
Files:
tests/ollama/test_schema.py
{src/draive,tests}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Target Python 3.12+ features and typing syntax
Files:
tests/ollama/test_schema.pysrc/draive/ollama/chat.py
docs/**
📄 CodeRabbit inference engine (CLAUDE.md)
docs/**: If behavior/API changes, update relevant docs under docs/ and examples as needed
When adding public APIs, update examples/guides and ensure cross-links render
Update documentation under docs/ when behavior/API changes
Files:
docs/getting-started/first-steps.mddocs/getting-started/quickstart.mddocs/getting-started/installation.mddocs/guides/EvaluatorCatalog.mddocs/cookbooks/BasicDataExtraction.md
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
When behavior/API changes, update relevant documentation pages under
docs/and examples as needed
Files:
docs/getting-started/first-steps.mddocs/getting-started/quickstart.mddocs/getting-started/installation.mddocs/guides/EvaluatorCatalog.mddocs/cookbooks/BasicDataExtraction.md
src/draive/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/draive/**/*.py: Import Haiway symbols directly, e.g., from haiway import State, ctx
Use ctx.scope(...) for context scoping and to avoid global state
All logs must use ctx.log_*; do not use print
Use strict typing with Python 3.12+ syntax; no untyped public APIs; avoid Any except at third‑party boundaries
Prefer explicit attribute access with static types; avoid dynamic getattr except at narrow boundaries
Prefer immutable protocols (Mapping, Sequence, Iterable) in public types over dict/list/set
Use final where applicable; avoid complex inheritance, prefer composition
Use precise unions (|) and narrow with match/isinstance; avoid cast unless provably safe and localized
Represent data/config and service facades as haiway.State; provide ergonomic constructors like .of(...)
Avoid in-place mutation of State; use State.updated(...) or functional builders
Access active state through haiway.ctx inside async scopes (ctx.scope(...))
Public state methods that dispatch on the active instance should use @statemethod
Record metrics using ctx.record where applicable
Prefer structured and concise log messages; avoid verbosity in hot paths
All I/O is async; keep boundaries async and use ctx.spawn for detached tasks
Rely on haiway and asyncio; avoid custom threading
Do not raise bare Exception; include contextual information in exceptions
Public symbols should have NumPy-style docstrings with Parameters/Returns/Raises
Avoid docstrings for internal helpers; keep names self-explanatory
Keep docstrings high-quality; mkdocstrings pulls them into reference pages
src/draive/**/*.py: Import Haiway symbols directly:from haiway import State, ctx
Usectx.scope(...)to bind scoped Disposables and active State; avoid global state
Route all logs throughctx.log_*; do not use
No untyped public APIs; maintain strict typing
Avoid looseAnyexcept at explicit third‑party boundaries
PreferMapping/Sequence/Iterableover concretedict/list/setin public types
Use `...
Files:
src/draive/ollama/chat.py
src/draive/{generation,models,openai,anthropic,mistral,gemini,vllm,ollama,bedrock,cohere}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Log around generation calls, tool dispatch, and provider requests/responses (without leaking secrets)
Files:
src/draive/ollama/chat.py
src/draive/{openai,anthropic,mistral,gemini,vllm,ollama,bedrock,cohere}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Translate provider/SDK errors into typed exceptions
Files:
src/draive/ollama/chat.py
src/draive/{openai,anthropic,mistral,gemini,vllm,ollama,bedrock,cohere,httpx}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Never log secrets or full request bodies containing keys/tokens
Files:
src/draive/ollama/chat.py
🧠 Learnings (1)
📚 Learning: 2025-10-01T13:12:39.701Z
Learnt from: CR
PR: miquido/draive#0
File: AGENTS.md:0-0
Timestamp: 2025-10-01T13:12:39.701Z
Learning: Run linters/type-checkers Ruff, Bandit, Pyright (strict) via `make lint`
Applied to files:
.github/workflows/ci.ymlMakefile
🧬 Code graph analysis (1)
tests/ollama/test_schema.py (1)
src/draive/ollama/chat.py (2)
_schema_for_ollama(392-402)_response_format(586-604)
🪛 checkmake (0.2.2)
Makefile
[warning] 16-16: Missing required phony target "all"
(minphony)
[warning] 16-16: Missing required phony target "clean"
(minphony)
🪛 markdownlint-cli2 (0.18.1)
docs/guides/EvaluatorCatalog.md
77-77: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (12)
pyproject.toml (1)
8-8: LGTM: Version bump aligns with documentation improvements.The version increment from 0.87.4 to 0.87.5 is appropriate for the documentation cleanup and linting workflow additions in this PR.
docs/guides/EvaluatorCatalog.md (1)
10-14: LGTM: Improved section structure.Adding "Evaluators" to section titles and introducing "Utility Evaluators" improves the organizational clarity of the overview.
llms.txt (1)
1-259: Excellent production-focused restructuring.The rewrite transforms this document from a framework reference into a production guide with clear emphasis on reliability, observability, and safety. The structured sections covering runtime environment, state management, and deployment practices provide actionable guidance for production deployments.
Preserving the original framework reference content in the "Extended Reference" section (line 260+) maintains backward compatibility while prioritizing production concerns.
docs/getting-started/installation.md (1)
1-53: Well-structured installation guide with progressive examples.The rewrite provides clear, step-by-step installation instructions that help users understand:
- Environment isolation requirements (Python 3.12+)
- Progressive installation (base → single provider → multi-provider)
- uv vs pip tradeoffs (deterministic installs)
- Optional extras and their purposes
- Verification before proceeding to quickstart
The progressive examples (lines 14-24) and separate uv section (lines 26-37) effectively guide users through different installation scenarios.
docs/getting-started/quickstart.md (1)
1-142: Comprehensive quickstart with excellent progressive examples.The expanded quickstart effectively demonstrates core Draive capabilities:
- Prerequisites and setup (lines 5-23): Proper environment configuration with
load_env()- Basic generation (lines 25-46): Simple text generation pattern
- Tool integration (lines 48-72): Function calling with
@tooldecorator- Structured output (lines 73-102): Type-safe data extraction with
DataModel- Provider swapping (lines 103-134): Multi-provider flexibility
The progression from simple to advanced patterns helps users understand Draive's capabilities while the typed
async def main() -> Nonesignature (line 32) demonstrates Python 3.12+ best practices.Makefile (3)
82-83: LGTM: docs-lint target provides local/CI parity.The new
docs-linttarget allows developers to run the same Markdown linting locally that CI enforces, improving the feedback loop and catching issues before push.
76-80: Good integration of docs linting into the main lint workflow.Adding the Markdown linter to the
linttarget ensures documentation quality is checked alongside code quality (Bandit, Ruff, Pyright). This provides comprehensive quality checks in a single command.
95-98: Excellent: --strict flag catches documentation build errors.Using
mkdocs build --clean --strictensures that documentation builds fail on warnings (broken links, missing pages, invalid references), catching issues that might otherwise go unnoticed in production.tests/ollama/test_schema.py (3)
24-30: Good test fixtures for schema normalization scenarios.The new
_OptionalStringand_UnsupportedUnionfixtures clearly separate:
- Supported nullable types (str | None) that can be normalized
- Unsupported union types (str | bool) that cannot be collapsed
This separation improves test clarity and validates the schema normalization logic in
src/draive/ollama/chat.py.
71-103: Comprehensive union and nullable type testing.These tests validate the schema normalization behavior for Ollama:
- Union collapse (lines 71-90): Verifies str|int unions collapse to "string" with debug logging
- Nullable scalar (lines 92-103): Confirms str|None fields are normalized to required "string" properties
The monkeypatching approach to capture debug logs ensures the normalization is tracked and logged correctly.
Note: A previous review suggested parametrizing these tests to cover additional cases like
{"number","integer"},{"object","null"}, and{"array","null"}. While the current tests are adequate, parametrization would reduce duplication and enable easier expansion of test coverage in the future.
105-119: LGTM: Updated tests use the correct fixtures.Switching from
_UnionSchemato_UnsupportedUnion(lines 106, 117) correctly validates that truly unsupported union types (str | bool) are rejected by the schema normalization logic and trigger appropriate fallback behavior..github/workflows/ci.yml (1)
34-36: LGTM: Docs linting integrated into CI workflow. Verified tools/markdown_lint.py exists with a main() entry point.
| if len(distinct) == 1: | ||
| value = next(iter(distinct)) | ||
| return "number" if value == "integer" else value | ||
|
|
||
| non_null: set[str] = distinct - {"null"} | ||
| if not non_null: | ||
| return "null" | ||
|
|
||
| if non_null <= {"number", "integer"}: | ||
| return "number" | ||
|
|
||
| if "string" in non_null and non_null <= {"string", "number", "integer"}: | ||
| return "string" |
There was a problem hiding this comment.
Don't widen integer-only unions to number.
Line 424 forces a lone "integer" type (or "integer"|"null") to collapse to "number", loosening schemas that should stay integer-only. For example, a DataModel field with type=["integer"] now accepts floats after normalization.
Apply this fix:
- if len(distinct) == 1:
- value = next(iter(distinct))
- return "number" if value == "integer" else value
+ if len(distinct) == 1:
+ return next(iter(distinct))
@@
- if non_null <= {"number", "integer"}:
+ if non_null == {"integer"}:
+ return "integer"
+
+ if non_null <= {"number", "integer"}:
return "number"📝 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.
| if len(distinct) == 1: | |
| value = next(iter(distinct)) | |
| return "number" if value == "integer" else value | |
| non_null: set[str] = distinct - {"null"} | |
| if not non_null: | |
| return "null" | |
| if non_null <= {"number", "integer"}: | |
| return "number" | |
| if "string" in non_null and non_null <= {"string", "number", "integer"}: | |
| return "string" | |
| if len(distinct) == 1: | |
| return next(iter(distinct)) | |
| non_null: set[str] = distinct - {"null"} | |
| if not non_null: | |
| return "null" | |
| if non_null == {"integer"}: | |
| return "integer" | |
| if non_null <= {"number", "integer"}: | |
| return "number" | |
| if "string" in non_null and non_null <= {"string", "number", "integer"}: | |
| return "string" |
🤖 Prompt for AI Agents
In src/draive/ollama/chat.py around lines 422 to 434, the normalization
currently collapses "integer" (and "integer"|"null") to "number", widening
integer-only schemas; change the logic so a lone "integer" returns "integer"
(not "number"), and when non_null is a subset of {"number","integer"} return
"number" only if "number" is present, otherwise return "integer" (so {"integer"}
-> "integer" and {"integer","null"} -> "integer"); keep the existing string
handling unchanged.
1 participant
No description provided.