Repository navigation
fix(api): repair enrich() data path + stop sending OpenAI-only kwarg to Anthropic - #188
Conversation
…g to Anthropic Three bugs shipped in 1.11.0, all found by installing the published wheel into a clean venv and calling the real APIs. None were visible to CI. 1. enrich(schema=...) was completely broken — ValueError: "Either dataframe or source_path must be provided", for BOTH DataFrame and CSV-path input. PipelineBuilder.from_specifications() copies the five spec objects but never carried the data, and QuickPipeline attaches an in-memory frame even for a path. The structured-output rebuild therefore always dropped it. from_specifications() now takes an explicit dataframe= argument. Missed by tests because test_enrich.py mocks Pipeline.execute, so the rebuilt pipeline never reached the loader stage — configuration was asserted, behaviour was not. 2. enrich(polars_df, ...) raised ModuleNotFoundError: No module named 'pyarrow'. polars is a core dependency and polars support is documented, but DataFrame.to_pandas() goes through Arrow and pyarrow ships only in the parquet/all extras. Both conversion directions now fall back to a column-wise copy that needs no extra dependency. Missed by tests because `uv sync --all-extras` installs pyarrow in the dev venv, masking the missing declaration. 3. Anthropic structured output failed with "AsyncMessages.create() got an unexpected keyword argument 'parallel_tool_calls'" (3 integration tests). instructor >=1.15 normalises from_anthropic(mode=ANTHROPIC_TOOLS) down to Mode.TOOLS, so the existing mode check matched the native Anthropic client and injected an OpenAI-only parameter its SDK rejects outright. The guard now excludes the direct-Anthropic path. Introduced by #182 raising instructor's floor from >=1.0.0 to >=1.15.4. Adds regression tests for (1) and (2) that exercise the real failure — both verified to fail without the fix and pass with it. Not fixed here, filed as #187: structured output has no mode fallback, and a run where every row fails is silently returned as a DataFrame of None. Verified: 1066 unit tests pass, mypy clean (126 files), ruff clean. Polars round-trip confirmed live against a pyarrow-free venv.
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change refines structured-output tool-call handling for native Anthropic clients. It also preserves dataframes during pipeline reconstruction and adds Arrow-independent Polars conversion paths with regression tests. ChangesStructured-output client handling
Dataframe preservation and conversion
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (2)
tests/unit/test_enrich.py (1)
359-376: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
_to_polarsfallback.This test only exercises
_polars_to_pandas. Simulateresult.to_polars()raisingModuleNotFoundErrorand assert that_to_polars()returns equivalent Polars data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_enrich.py` around lines 359 - 376, Extend test_polars_conversion_without_pyarrow to also cover _to_polars: monkeypatch result.to_polars() to raise ModuleNotFoundError, invoke _to_polars() with representative data, and assert it returns equivalent Polars data with the expected columns and values.ondine/adapters/unified_litellm_client.py (1)
987-996: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the excluded native-Anthropic path.
The provided regression test only verifies the positive case: LiteLLM-backed TOOLS mode sends
parallel_tool_calls=False. No test verifies the negative case that this change fixes: a native Anthropic Instructor client (_uses_direct_anthropic_instructor=True) must NOT receiveparallel_tool_callsincall_kwargs. Add a test that constructs a client withprovider="anthropic"and no router, spies oninstructor_client.create, and assertsparallel_tool_callsis absent from the captured kwargs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ondine/adapters/unified_litellm_client.py` around lines 987 - 996, Add a regression test covering the native Anthropic path in the client construction/test suite: create the client with provider="anthropic" and no router, spy on instructor_client.create, invoke the relevant completion flow in TOOLS mode, and assert the captured call_kwargs do not contain parallel_tool_calls. Keep the existing LiteLLM positive-case test unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ondine/adapters/unified_litellm_client.py`:
- Around line 987-996: Add a regression test covering the native Anthropic path
in the client construction/test suite: create the client with
provider="anthropic" and no router, spy on instructor_client.create, invoke the
relevant completion flow in TOOLS mode, and assert the captured call_kwargs do
not contain parallel_tool_calls. Keep the existing LiteLLM positive-case test
unchanged.
In `@tests/unit/test_enrich.py`:
- Around line 359-376: Extend test_polars_conversion_without_pyarrow to also
cover _to_polars: monkeypatch result.to_polars() to raise ModuleNotFoundError,
invoke _to_polars() with representative data, and assert it returns equivalent
Polars data with the expected columns and values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 60ee4ab1-3dfa-499e-b41e-8d1650c9b413
📒 Files selected for processing (4)
ondine/adapters/unified_litellm_client.pyondine/api/enrich.pyondine/api/pipeline_builder.pytests/unit/test_enrich.py
CI resolves ruff 0.16.0 (floor is >=0.15.21); the appended regression test class was formatted under an older local resolution.
1 participant
Three bugs shipped in 1.11.0, all found by installing the published wheel into a clean venv and calling the real APIs. None were visible to CI.
1.
enrich(schema=...)was completely brokenFailed for both DataFrame and CSV-path input — i.e. the entire structured-output half of the flagship new API.
PipelineBuilder.from_specifications()copies the five spec objects but never carried the data, andQuickPipelineattaches an in-memory frame even when given a path. So the structured-output rebuild always dropped it.from_specifications()now takes an explicitdataframe=argument.Why CI missed it:
test_enrich.pymocksPipeline.execute, so the rebuilt pipeline never reached the loader stage. The tests asserted configuration and never exercised behaviour.2.
enrich(polars_df, ...)crashed on a default installpolarsis a core dependency and polars support is documented, butDataFrame.to_pandas()goes through Arrow andpyarrowships only in theparquet/allextras. Both conversion directions now fall back to a column-wise copy needing no extra dependency.Why CI missed it:
uv sync --all-extrasinstalls pyarrow in the dev venv, masking the missing declaration.3. Anthropic structured output broken
This was the cause of the 3 failing integration tests. instructor ≥1.15 normalises
from_anthropic(mode=ANTHROPIC_TOOLS)down toMode.TOOLS, so the existing mode check matched the native Anthropic client and injected an OpenAI-only parameter its SDK rejects outright. The guard now excludes the direct-Anthropic path.Introduced by #182 raising instructor's floor from
>=1.0.0to>=1.15.4.Verification
Honest caveats: (3) is proven by root-cause analysis and unit tests, not a live call — the integration model
claude-3-haiku-20240307is retired (404).enrich(schema=)could not be confirmed end-to-end either, because no free model I had access to supports structured output; the data-path fix is proven structurally (rebuilt.dataframe is not None).Not fixed here — see #187
Structured output has no mode fallback (the working mode differs per model:
ling-3.0-flashneedsTOOLS, DeepSeek rejects bothTOOLSandJSON_SCHEMA),deepseekis missing fromPROVIDER_CAPABILITIESentirely, and a run where every row fails is silently returned as a DataFrame ofNone. That last part is the same failure class as #166 and is behaviour-changing, so it doesn't belong in a patch.Targets 1.11.1.
Summary by CodeRabbit