Repository navigation
Remove instructor - #105
Conversation
Reviewer's GuideThis PR removes the Instructor dependency, switches LLM interaction to native LiteLLM structured responses with schema validation and retry logic, propagates API base URL and max retries configuration through the pipeline, and tightens the pydantic schema for additional_parameters to a structured list model. Sequence diagram for LiteLLM structured extraction with validation-based retriessequenceDiagram
actor NomadPipeline
participant llm_call as create_text_completion
participant LiteLLM as completion
participant Schema as PerovskiteSolarCells
NomadPipeline->>llm_call: create_text_completion(model_name, pdf_text, api_key, api_base_url)
llm_call->>LiteLLM: completion(model, base_url, api_key, max_tokens, messages, response_format)
LiteLLM-->>llm_call: resp
loop retry until max_retries
llm_call->>Schema: PerovskiteSolarCells.model_validate_json(resp.choices[0].message.content)
alt [validation passes]
Schema-->>llm_call: extracted_data
llm_call-->>NomadPipeline: (extracted_data, resp)
else [ValidationError]
llm_call->>llm_call: increment retry_count
alt [retry_count >= max_retries]
llm_call-->>NomadPipeline: MaxRetriesExceededError(resp)
else [retry_count < max_retries]
llm_call->>llm_call: extend messages with previous content and retry_prompt
llm_call->>LiteLLM: completion(..., messages, response_format)
LiteLLM-->>llm_call: resp
end
end
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR replaces Instructor completion handling with direct LiteLLM calls, adds strict validation and retries, forwards additional parameters through the pipeline, improves failure recovery and logging, updates processing parameter models, and adds model-sweep automation. ChangesLLM extraction flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The retry logic in
create_text_completionresetsretry_countto 0 on every loop iteration, which preventsmax_retriesfrom ever being reached and could cause unbounded retries; consider movingretry_countoutside thewhile Trueand incrementing it per request. - In
create_text_completion, raisingMaxRetriesExceededError(resp)is immediately followed by abreakthat is never executed; you can simplify this branch by removing the unreachablebreakor restructuring the control flow. - The call to
completionwithresponse_format=PerovskiteSolarCellsdepends on LiteLLM's schema support; ensure this aligns with the expectedresponse_formatstructure (e.g., JSON schema or tool-calls) so the model output is compatible withPerovskiteSolarCells.model_validate_json.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The retry logic in `create_text_completion` resets `retry_count` to 0 on every loop iteration, which prevents `max_retries` from ever being reached and could cause unbounded retries; consider moving `retry_count` outside the `while True` and incrementing it per request.
- In `create_text_completion`, raising `MaxRetriesExceededError(resp)` is immediately followed by a `break` that is never executed; you can simplify this branch by removing the unreachable `break` or restructuring the control flow.
- The call to `completion` with `response_format=PerovskiteSolarCells` depends on LiteLLM's schema support; ensure this aligns with the expected `response_format` structure (e.g., JSON schema or tool-calls) so the model output is compatible with `PerovskiteSolarCells.model_validate_json`.
## Individual Comments
### Comment 1
<location path="src/perla_extract/llm_call.py" line_range="99-100" />
<code_context>
-
+ print(f"Could not fetch model info, defaulting to max_tokens={max_tokens}. Error: {e}")
+
while True:
+ retry_count = 0
try:
- resp, compll = client.chat.completions.create_with_completion(
</code_context>
<issue_to_address>
**issue (bug_risk):** Retry counter is reset on each loop iteration, preventing the max_retries guard from ever being reached.
Since `retry_count` is set to 0 inside the `while True` loop, it never accumulates across iterations and the `retry_count >= max_retries` condition is never satisfied. Move the `retry_count` initialization outside the loop and increment it only in the `except ValidationError` block. You can also remove the `break` after `raise MaxRetriesExceededError`, as it is unreachable.
</issue_to_address>
### Comment 2
<location path="src/perla_extract/llm_call.py" line_range="120" />
<code_context>
print("reduced max tokens")
- else:
+ try:
+ extracted_data = PerovskiteSolarCells.model_validate_json(resp.choices[0].message.content,strict=True,extra="forbid")
break
- return resp, compll
</code_context>
<issue_to_address>
**issue (bug_risk):** Use of `extra="forbid"` with `model_validate_json` is likely invalid for Pydantic v2.
`model_validate_json` in Pydantic v2 does not accept an `extra` keyword argument, so this call will raise a `TypeError`. To forbid extra fields, configure `model_config = ConfigDict(extra="forbid")` (or equivalent) on `PerovskiteSolarCells` and remove the `extra` parameter from `model_validate_json`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/perla_extract/llm_call.py (1)
44-64: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale docstring and comment referencing Instructor.
The docstring still says the function sends prompts via the
instructorlibrary and LiteLLM backend and listsinstructor.exceptions.InstructorRetryExceptionunderRaises:. The# Call with Instructorcomment at line 73 is also stale. The function now calls LiteLLM directly and raisesValidationError/MaxRetriesExceededError, not any Instructor exception. Update the docstring and comment to match the current implementation to avoid misleading future maintainers debugging the retry flow.Also applies to: 73-73
🤖 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 `@src/perla_extract/llm_call.py` around lines 44 - 64, Update the function docstring to describe direct LiteLLM usage and document the actual ValidationError and MaxRetriesExceededError outcomes instead of Instructor APIs; also replace the stale “Call with Instructor” comment near the LiteLLM invocation with wording that reflects the current call path.
🤖 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.
Inline comments:
In `@src/perla_extract/llm_call.py`:
- Around line 99-131: Move retry_count initialization outside the while True
loop so validation retries are bounded by max_retries, and in the context-limit
BadRequestError branch after reducing max_tokens, continue immediately to retry
completion before accessing resp. Preserve the existing re-raise behavior for
other BadRequestError instances and the validation retry flow.
In `@src/perla_extract/pipeline.py`:
- Line 344: Update the result unpacking in extract_from_pdf_nomad for the
llm_call.create_text_completion call by renaming the unused completion_usage
variable with a leading underscore, preserving the existing completion result
handling.
---
Outside diff comments:
In `@src/perla_extract/llm_call.py`:
- Around line 44-64: Update the function docstring to describe direct LiteLLM
usage and document the actual ValidationError and MaxRetriesExceededError
outcomes instead of Instructor APIs; also replace the stale “Call with
Instructor” comment near the LiteLLM invocation with wording that reflects the
current call path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 79aede4e-8c00-4023-92b5-30416126aa6e
📒 Files selected for processing (5)
pyproject.tomlsrc/perla_extract/configuration.pysrc/perla_extract/llm_call.pysrc/perla_extract/pipeline.pysrc/perla_extract/pydantic_model_reduced.py
There was a problem hiding this comment.
Pull request overview
This PR targets issue #96 by migrating the extraction flow away from Instructor toward direct LiteLLM usage, adding validation-driven retry behavior, and tightening the schema for processing-step additional_parameters to better align with OpenAI-compatible structured outputs.
Changes:
- Introduces
AdditionalParameterand changesProcessingStep.additional_parametersfrom a free-formdictto a structuredList[AdditionalParameter]. - Refactors
create_text_completionto call LiteLLM directly, adds validation-error retry logic, and introducesMaxRetriesExceededError. - Adds
max_retriesto configuration and bumpslitellmdependency to>=1.93.0; adds optionalapi_base_urlplumbing for NOMAD extraction.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/perla_extract/pydantic_model_reduced.py | Adds AdditionalParameter model and changes additional_parameters to a structured list. |
| src/perla_extract/pipeline.py | Plumbs api_base_url, handles MaxRetriesExceededError, and adjusts failure parsing for LiteLLM responses. |
| src/perla_extract/llm_call.py | Reworks extraction to use LiteLLM directly and adds validation-driven retry behavior. |
| src/perla_extract/configuration.py | Adds a max_retries configuration value. |
| pyproject.toml | Upgrades LiteLLM version requirement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| print("reduced max tokens") | ||
| else: | ||
| try: | ||
| extracted_data = PerovskiteSolarCells.model_validate_json(resp.choices[0].message.content,strict=True,extra="forbid") |
There was a problem hiding this comment.
There was a problem hiding this comment.
We ignore extra fields during evals anyways. Do we even need this?
There was a problem hiding this comment.
We can remove this once this PR is merged:
BerriAI/litellm#36082
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/perla_extract/pipeline.py (3)
339-350: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
extract_from_pdf_nomaddropsself.additional_params.
_extract_pdfforwardsadditional_params=self.additional_paramstocreate_text_completion(Line 368), butextract_from_pdf_nomadonly forwardsapi_keyandapi_base_url(Lines 346-348). Any additional parameters configured on the pipeline are silently ignored for the NOMAD extraction path, producing inconsistent behavior between the two extraction entry points.🐛 Proposed fix
results, completion_usage = llm_call.create_text_completion( - self.model_name, pdf_text, api_key=api_key, api_base_url=api_base_url + self.model_name, + pdf_text, + api_key=api_key, + api_base_url=api_base_url, + additional_params=self.additional_params, )🤖 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 `@src/perla_extract/pipeline.py` around lines 339 - 350, Update extract_from_pdf_nomad to forward self.additional_params when calling llm_call.create_text_completion, matching the existing _extract_pdf invocation while preserving the current API key and base URL arguments.
366-389: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftUnhandled LLM errors can crash the whole batch.
_extract_pdfonly catchesValidationError,MaxRetriesExceededError, andjson.decoder.JSONDecodeError.create_text_completioncan raise other litellm exceptions, for example aBadRequestErrorwhose message does not match the context-limit string is re-raised as-is, along with rate-limit, timeout, or other API errors. Since_extract_batchcalls_extract_pdfin a loop over all PDFs, an unhandled exception from one file aborts the extraction of every remaining file in the batch, instead of failing just that one file.Add a catch-all around the completion call (or a broader except clause here) that logs the failure and lets the loop continue.
🤖 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 `@src/perla_extract/pipeline.py` around lines 366 - 389, Update _extract_pdf to catch unexpected exceptions from create_text_completion in addition to the existing specific exceptions, log the failure using the established failure-handling path, and return False so _extract_batch continues processing subsequent PDFs.
314-337: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winDefault
additional_params=Nonecrashescreate_text_completionon the default path.
ExtractionPipeline.__init__(Line 323) andextract()(Line 498) both defaultadditional_paramstoNone. ThatNoneis stored and later passed explicitly asadditional_params=self.additional_paramsat Line 368. This overridescreate_text_completion's ownadditional_params: dict = {}default, so when no additional parameters are supplied,Nonereaches thefor param in additional_params:loop inllm_call.create_text_completionand raisesTypeError: 'NoneType' object is not iterable. Every default-path extraction call will fail before the completion request is even made.Default to an empty dict, or normalize
Noneto{}before forwarding.🐛 Proposed fix
def __init__( self, model_name: str, preprocessor: str, postprocessor: str, cache_dir: Union[Path, str], use_cache: bool = True, nomad: bool = False, nomad_upload_id: str = None, - additional_params: dict = None, + additional_params: Optional[dict] = None, ): ... - self.additional_params = additional_params + self.additional_params = additional_params or {}Also applies to: 366-369, 486-516
🤖 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 `@src/perla_extract/pipeline.py` around lines 314 - 337, Normalize additional_params to an empty dictionary in ExtractionPipeline.__init__ (and the extract() entry point) before storing or forwarding it, so the additional_params passed to create_text_completion is always iterable when callers omit it or provide None.src/perla_extract/llm_call.py (3)
43-64: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the public docstring for the new API contract.
The function now returns
(PerovskiteSolarCells, raw_response)and can raiseMaxRetriesExceededError. The docstring still documents Instructor, a single model return, andInstructorRetryException. Addapi_base_urlandadditional_paramstoArgs.🤖 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 `@src/perla_extract/llm_call.py` around lines 43 - 64, Update the public docstring for the affected extraction function to document its tuple return value as (PerovskiteSolarCells, raw_response), replace the outdated InstructorRetryException with MaxRetriesExceededError in Raises, remove obsolete implementation-specific Instructor wording, and add api_base_url and additional_params to Args.
118-126: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winBound context-length retries before the token budget becomes invalid.
This branch decreases
max_tokensby 5000 and continues without a lower bound or separate retry limit. If the prompt still exceeds the context window, it can issue requests indefinitely and eventually send a non-positivemax_tokens.Re-raise when the next reduction would be invalid, or use a bounded context-retry counter.
Proposed guard
except litellm.exceptions.BadRequestError as e: ... + if max_tokens <= 5000: + raise max_tokens -= 5000 print("reduced max tokens") continue🤖 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 `@src/perla_extract/llm_call.py` around lines 118 - 126, Update the BadRequestError handling in the LLM request retry loop to bound context-length retries: before subtracting 5000 from max_tokens, ensure the next value remains positive and the retry count remains within a finite limit; otherwise re-raise the original error. Preserve the existing behavior for non-context-limit errors and valid reductions.
118-123: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse the LiteLLM context-window exception instead of string-matching one provider message.
Catch
litellm.ContextWindowExceededError(or another supported structured context-window signal) so context recovery works when LiteLLM maps the provider error, instead of depending on one exact Anthropic error string.🤖 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 `@src/perla_extract/llm_call.py` around lines 118 - 123, Update the exception handling around the LLM call to catch LiteLLM’s structured ContextWindowExceededError (or the supported structured context-window exception) instead of filtering BadRequestError by an exact Anthropic message string. Preserve re-raising unrelated exceptions while allowing the existing context-recovery path to handle recognized context-window failures.
🧹 Nitpick comments (3)
src/perla_extract/pipeline.py (1)
451-462: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
loggerinstead of
_handle_failureusesprint(f"Extraction failed: {error}"), whilecreate_text_completion(per the upstreamllm_call.pycontract) useslogger.error/logger.warning. Route this failure message through the same logger for consistent, filterable diagnostics across the extraction flow.🤖 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 `@src/perla_extract/pipeline.py` around lines 451 - 462, Update _handle_failure to report extraction failures through the existing logger at error level instead of print, preserving the current message content and fallback processing behavior.src/perla_extract/llm_call.py (2)
102-103: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow the metadata fallback exception.
except Exceptioncovers LiteLLM metadata lookups, parameter filtering, and unrelated programming errors. It then prints and continues with a default token limit. Catch only expected metadata exceptions and log them throughlogger; let local errors propagate.🤖 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 `@src/perla_extract/llm_call.py` around lines 102 - 103, Update the exception handling around the model metadata lookup in the relevant function to catch only the expected LiteLLM metadata exception type, rather than broad Exception. Replace the print call with the existing logger while retaining the default max_tokens fallback for that expected failure, and allow parameter-filtering or other local programming errors to propagate.Source: Linters/SAST tools
41-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the hidden optional types and avoid a shared default mapping.
api_base_urlisNonebut annotated asstr.additional_params={}is mutable and can be shared across calls. Since the project targets Python 3.10+, usetyping.Optionalfor the optional parameters, and initializeadditional_paramsfromNonebefore mutating it.🤖 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 `@src/perla_extract/llm_call.py` around lines 41 - 42, Update the affected function signature in llm_call to annotate api_base_url with Optional[str] and replace the mutable additional_params default with None, then initialize it to a new dictionary inside the function before any mutation.Source: Linters/SAST tools
🤖 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.
Inline comments:
In `@model_sweep.sh`:
- Around line 21-36: Fix the extraction loop by wiring the command in the
`NORMAL_MODELS` iteration and checking its exit status immediately, or remove
the loop until extraction is implemented; do not evaluate `$?` after unrelated
commands such as `echo`, and preserve accurate success or failure reporting for
each model.
In `@src/perla_extract/llm_call.py`:
- Line 116: Update the additional_params expansion in the LLM call to handle
reserved completion keywords explicitly: model, base_url, api_key, max_tokens,
messages, response_format, and drop_params. Reject or remove these names before
expanding additional_params so the request never raises a duplicate-keyword
TypeError, while preserving all non-reserved configurable parameters.
- Around line 96-101: Update the parameter filtering logic in the LLM call flow
to avoid mutating additional_params while iterating over it. Build a filtered
mapping containing only supported parameters, log each ignored parameter, and
pass filtered_params to completion while leaving the caller’s original mapping
unchanged.
- Line 41: Update create_text_completion so the configured api_base_url is
passed to litellm.completion using LiteLLM’s api_base parameter instead of
base_url, ensuring NOMAD and other custom endpoint settings reach the main
pipeline.
In `@src/perla_extract/optimizer.py`:
- Line 142: Update the unpacking assignment in the optimizer flow around
llm_call.create_text_completion to bind the unused completion return value to an
underscore-prefixed variable, while preserving the existing results handling.
---
Outside diff comments:
In `@src/perla_extract/llm_call.py`:
- Around line 43-64: Update the public docstring for the affected extraction
function to document its tuple return value as (PerovskiteSolarCells,
raw_response), replace the outdated InstructorRetryException with
MaxRetriesExceededError in Raises, remove obsolete implementation-specific
Instructor wording, and add api_base_url and additional_params to Args.
- Around line 118-126: Update the BadRequestError handling in the LLM request
retry loop to bound context-length retries: before subtracting 5000 from
max_tokens, ensure the next value remains positive and the retry count remains
within a finite limit; otherwise re-raise the original error. Preserve the
existing behavior for non-context-limit errors and valid reductions.
- Around line 118-123: Update the exception handling around the LLM call to
catch LiteLLM’s structured ContextWindowExceededError (or the supported
structured context-window exception) instead of filtering BadRequestError by an
exact Anthropic message string. Preserve re-raising unrelated exceptions while
allowing the existing context-recovery path to handle recognized context-window
failures.
In `@src/perla_extract/pipeline.py`:
- Around line 339-350: Update extract_from_pdf_nomad to forward
self.additional_params when calling llm_call.create_text_completion, matching
the existing _extract_pdf invocation while preserving the current API key and
base URL arguments.
- Around line 366-389: Update _extract_pdf to catch unexpected exceptions from
create_text_completion in addition to the existing specific exceptions, log the
failure using the established failure-handling path, and return False so
_extract_batch continues processing subsequent PDFs.
- Around line 314-337: Normalize additional_params to an empty dictionary in
ExtractionPipeline.__init__ (and the extract() entry point) before storing or
forwarding it, so the additional_params passed to create_text_completion is
always iterable when callers omit it or provide None.
---
Nitpick comments:
In `@src/perla_extract/llm_call.py`:
- Around line 102-103: Update the exception handling around the model metadata
lookup in the relevant function to catch only the expected LiteLLM metadata
exception type, rather than broad Exception. Replace the print call with the
existing logger while retaining the default max_tokens fallback for that
expected failure, and allow parameter-filtering or other local programming
errors to propagate.
- Around line 41-42: Update the affected function signature in llm_call to
annotate api_base_url with Optional[str] and replace the mutable
additional_params default with None, then initialize it to a new dictionary
inside the function before any mutation.
In `@src/perla_extract/pipeline.py`:
- Around line 451-462: Update _handle_failure to report extraction failures
through the existing logger at error level instead of print, preserving the
current message content and fallback processing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 54318890-01e3-4b51-aa60-f3b7f0213e4f
📒 Files selected for processing (4)
model_sweep.shsrc/perla_extract/llm_call.pysrc/perla_extract/optimizer.pysrc/perla_extract/pipeline.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
model_sweep.sh (2)
30-34: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn a non-zero status when any extraction fails.
The script reports failures but does not retain them. The final
echothen returns status0, so an incomplete sweep appears successful to CI or automation.Track a
failedflag in bothelsebranches and exit with that flag after the final message.🛠️ Proposed fix
+failed=0 + for model in "${NORMAL_MODELS[@]}"; do ... else echo "❌ Failed to extract $model" + failed=1 fi ... else echo "❌ Failed to extract $model with reasoning effort: $effort" + failed=1 fi ... echo "All extractions complete!" +exit "$failed"Also applies to: 53-57, 61-61
🤖 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 `@model_sweep.sh` around lines 30 - 34, Update the extraction status handling around both success/failure branches to initialize and set a failed flag whenever an extraction fails, then emit the final message and exit using that flag so any failed extraction returns a non-zero status while fully successful sweeps return zero.
5-5: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the full OpenRouter model identifier.
LiteLLM calls OpenRouter provider models as
openrouter/<provider>/<model>, and OpenRouter exposes this model asopenai/gpt-oss-120b. The current value omitsopenai/and may fail model resolution.🐛 Proposed fix
- "openrouter/gpt-oss-120b" + "openrouter/openai/gpt-oss-120b"🤖 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 `@model_sweep.sh` at line 5, Update the model identifier in model_sweep.sh to use the full OpenRouter format, changing the current gpt-oss-120b entry to include the openai provider segment while preserving the existing openrouter prefix.
🤖 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.
Inline comments:
In `@src/perla_extract/llm_call.py`:
- Around line 96-117: Initialize filtered_params to an empty dictionary before
the try block that calls get_model_info or get_supported_openai_params, so the
exception fallback can safely proceed with the default max_tokens value and the
later filtered_params.update call remains valid.
---
Outside diff comments:
In `@model_sweep.sh`:
- Around line 30-34: Update the extraction status handling around both
success/failure branches to initialize and set a failed flag whenever an
extraction fails, then emit the final message and exit using that flag so any
failed extraction returns a non-zero status while fully successful sweeps return
zero.
- Line 5: Update the model identifier in model_sweep.sh to use the full
OpenRouter format, changing the current gpt-oss-120b entry to include the openai
provider segment while preserving the existing openrouter prefix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b695f4a-8be4-4090-b55b-4621a8ab5115
📒 Files selected for processing (2)
model_sweep.shsrc/perla_extract/llm_call.py
| except litellm.exceptions.BadRequestError as e: | ||
| if ( | ||
| 'litellm.BadRequestError: AnthropicException - {"type":"error","error":{"type":"invalid_request_error","message":"input length and `max_tokens` exceed context limit:' | ||
| 'AnthropicException - {"type":"error","error":{"type":"invalid_request_error","message":"input length and `max_tokens` exceed context limit:' |
There was a problem hiding this comment.
do we really need this hack?
There was a problem hiding this comment.
I kept it as is from what sherjeel had not sure if its still needed or not though
| "openai/gpt-5.6-sol" | ||
| "openai/gpt-5.6-terra" | ||
| ) | ||
| REASONING_EFFORT=( |
There was a problem hiding this comment.
is this always called the same way?
There was a problem hiding this comment.
I dont get what you mean? The lists of the models can be changed
There was a problem hiding this comment.
I believe we should include all the models that @sherjeelshabih has in the plots he posted on discord
There was a problem hiding this comment.
These contain the ones he had there except kimi I think. but he can modify it later to have all the final ones he chooses.
| from pydantic import BaseModel, Field | ||
| from pydantic import BaseModel, Field, ValidationError | ||
| from perla_extract.pydantic_model_reduced import PerovskiteSolarCells | ||
| from perla_extract.constants import SYSTEM_PROMPT, INSTRUCTION_TEXT |
There was a problem hiding this comment.
Maybe worth a separate PR, but I think we should investigate dropping the schema from the prompt
@sherjeelshabih introduced it for some Gemini model and it might even hurt performance
|
Thanks, @pillar12 for pushing this! I feel that there are still some leftovers that we should clean up before merging |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/perla_extract/llm_call.py (3)
124-126: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winBound the context-limit retry loop.
If the same error persists,
max_tokensdecreases by 5000 on every iteration and the loop continues indefinitely.max_tokenscan become zero or negative, andmax_retriesdoes not limit this branch. Stop and re-raise once no positive token budget remains.Proposed fix
- max_tokens -= 5000 + if max_tokens <= 1: + raise + max_tokens = max(1, max_tokens - 5000) print("reduced max tokens") continue🤖 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 `@src/perla_extract/llm_call.py` around lines 124 - 126, Bound the context-limit retry branch in the surrounding LLM call retry loop: after reducing max_tokens, stop retrying and re-raise the original error when no positive token budget remains. Ensure this path cannot continue with zero or negative max_tokens, while preserving retries when the reduced budget is still positive.
162-169: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the same structured-output completion pattern as extraction.
Pass
api_keyandapi_baseintocompletion/llm_as_judgeif extraction uses caller-supplied credentials, or ensure these value sources are consistent with the extraction path. Also handle invalid or empty JSON before callingJudgement.model_validate_json, since the previous Instructor exception path is no longer present.🤖 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 `@src/perla_extract/llm_call.py` around lines 162 - 169, Update the completion flow around llm_as_judge and the Judgement.model_validate_json call to match the extraction path’s structured-output pattern, using the same caller-supplied api_key and api_base sources. Validate that the completion content is present and valid JSON before strict Judgement validation, and handle invalid or empty responses explicitly rather than relying on the removed Instructor exception path.
118-123: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake the context-limit retry provider-neutral.
This path can receive any LiteLLM model; relying on one serialized Anthropic error string misses context-window errors from other providers and re-raises them instead of reducing
max_tokens. Inspect the LiteLLM context-window exception/error contract and route provider-specific context errors through the same retry path, including tests for each model family used in the sweep.🤖 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 `@src/perla_extract/llm_call.py` around lines 118 - 123, Update the BadRequestError handling in the LLM call flow to detect LiteLLM’s provider-neutral context-window error contract instead of matching only the serialized Anthropic message. Route context-limit failures from every supported model family through the existing max_tokens-reduction retry path, while re-raising unrelated bad requests, and add coverage for each model family used in the sweep.
🤖 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.
Outside diff comments:
In `@src/perla_extract/llm_call.py`:
- Around line 124-126: Bound the context-limit retry branch in the surrounding
LLM call retry loop: after reducing max_tokens, stop retrying and re-raise the
original error when no positive token budget remains. Ensure this path cannot
continue with zero or negative max_tokens, while preserving retries when the
reduced budget is still positive.
- Around line 162-169: Update the completion flow around llm_as_judge and the
Judgement.model_validate_json call to match the extraction path’s
structured-output pattern, using the same caller-supplied api_key and api_base
sources. Validate that the completion content is present and valid JSON before
strict Judgement validation, and handle invalid or empty responses explicitly
rather than relying on the removed Instructor exception path.
- Around line 118-123: Update the BadRequestError handling in the LLM call flow
to detect LiteLLM’s provider-neutral context-window error contract instead of
matching only the serialized Anthropic message. Route context-limit failures
from every supported model family through the existing max_tokens-reduction
retry path, while re-raising unrelated bad requests, and add coverage for each
model family used in the sweep.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0dd2630b-3e1e-413e-9d40-2ced0b6c4446
📒 Files selected for processing (1)
src/perla_extract/llm_call.py
Co-authored-by: Kevin M Jablonka <32935233+kjappelbaum@users.noreply.github.com>
3465a33 to
085c703
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/perla_extract/pipeline.py (1)
346-348: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winForward
additional_paramsin NOMAD extraction.
_extract_pdfforwardsself.additional_params, butextract_from_pdf_nomaddoes not. NOMAD calls therefore ignore configured parameters such asreasoning_effort.results, completion_usage = llm_call.create_text_completion( - self.model_name, pdf_text, api_key=api_key, api_base_url=api_base_url + self.model_name, + pdf_text, + api_key=api_key, + api_base_url=api_base_url, + additional_params=self.additional_params, )🤖 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 `@src/perla_extract/pipeline.py` around lines 346 - 348, Update extract_from_pdf_nomad’s call to llm_call.create_text_completion so it forwards self.additional_params, matching the existing _extract_pdf behavior and ensuring configured options such as reasoning_effort are applied.src/perla_extract/llm_call.py (1)
82-108: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNormalize
additional_paramsand preserve it for the fallback path.If
create_text_completionreceivesadditional_params=None, it raisesAttributeErrorat.items(). If the model capability lookup raises, the exception block skips copying requested parameters, so values likereasoning_effortare never passed tocompletion. Store a normalized copy of the requested parameters before attempting the lookup, keep those parameters in the fallback, and only apply supported-parameter filtering when the lookup succeeds.🤖 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 `@src/perla_extract/llm_call.py` around lines 82 - 108, Normalize additional_params to an empty mapping when create_text_completion receives None, and copy all requested parameters into filtered_params before the model capability lookup. In the successful lookup path, replace that copy with supported-parameter filtering; if the lookup raises, retain the unfiltered parameters so they still reach completion.
🤖 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.
Inline comments:
In `@src/perla_extract/llm_call.py`:
- Around line 121-129: Update the BadRequestError handling in the LLM retry loop
to use LiteLLM’s provider-neutral context-limit exception path instead of
matching Anthropic-specific text. Only reduce max_tokens by 5000 and retry when
max_tokens is greater than zero; otherwise propagate the error, preserving
existing behavior for unrelated bad requests.
---
Outside diff comments:
In `@src/perla_extract/llm_call.py`:
- Around line 82-108: Normalize additional_params to an empty mapping when
create_text_completion receives None, and copy all requested parameters into
filtered_params before the model capability lookup. In the successful lookup
path, replace that copy with supported-parameter filtering; if the lookup
raises, retain the unfiltered parameters so they still reach completion.
In `@src/perla_extract/pipeline.py`:
- Around line 346-348: Update extract_from_pdf_nomad’s call to
llm_call.create_text_completion so it forwards self.additional_params, matching
the existing _extract_pdf behavior and ensuring configured options such as
reasoning_effort are applied.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e43d2f8-2788-4581-9cf4-4556441c8dd5
📒 Files selected for processing (3)
src/perla_extract/configuration.pysrc/perla_extract/llm_call.pysrc/perla_extract/pipeline.py
| print("reduced max tokens") | ||
| else: | ||
| try: | ||
| extracted_data = PerovskiteSolarCells.model_validate_json(resp.choices[0].message.content,strict=True,extra="forbid") |
There was a problem hiding this comment.
We ignore extra fields during evals anyways. Do we even need this?
#96
Summary by Sourcery
Replace Instructor-based structured extraction with direct LiteLLM usage and add robust validation-driven retry handling for LLM outputs.
New Features:
Bug Fixes:
Enhancements:
Summary by CodeRabbit
New Features
Bug Fixes
Chores