Extract evaluator LLM provider references into FK fields - #3999
Conversation
|
Warning Review limit reached
Next review available in: 19 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
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 |
SmittieC
left a comment
There was a problem hiding this comment.
LGTM. A few clarifying questions only
The ids lived only in the params JSON, so a deleted provider left a dangling integer. params stays what the schema-driven form edits; the FK becomes the reference the runtime resolves. Closes #3977 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR CI runs pytest with --no-migrations, which leaves MigrationLoader/ MigrationExecutor with no graph to load. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4d04cdf to
a0c0620
Compare
Closes #3977. Part 2 is #3995. Supersedes #3992, which carried this plus four follow-ons.
#3998 has merged; this now targets
main.Product Description
An evaluator whose LLM provider or model has been deleted now says so — "has no LLM provider configured, edit the evaluator and select a provider and model" — instead of failing mid-run with a bare lookup error.
Technical Description
Evaluator.paramsheldllm_provider_id/llm_provider_model_idas plain integers, so deleting a provider left a dangling id (see #3974, and one such row in prod today). These become real FKs withSET_NULL.paramskeeps its copy of both ids: the evaluator form is generated from the pydantic schema inevaluators.py, so the ids have to stay in the schema for the UI to edit them. The FK is the authoritative reference —Evaluator.savederives the columns fromparamson every write (dangling ids resolve to null rather than tripping the constraint) andget_evaluator_params()reads them back off the FKs at run time. Deriving insaverather than in the form is what keeps cloning, bootstrap data, and factories consistent without each remembering. Same shape asNode._sync_resource_fk_fields.get_related_evaluators_querysetis gone — the usages page reaches evaluators through the reverse FK now.Two consequences worth calling out, because they have no corresponding line of code:
LlmProviderModelan evaluator uses is now blocked, the moment the reverse FK exists —get_candidate_relations_to_deletepicks it up and the existing guard does the rest. The flows that legitimately need the model gone (_replace_custom_model_with_global,remove_deprecated_models) repoint evaluators first.has_related_objects._replace_custom_model_with_globalalso runs from migrations, whereEvaluatormay be absent from the app state entirely. It raises unless the FK column is genuinely not in the database yet, so a future migration missing theevaluations.0018dependency fails with instructions rather than a deferred FK violation at commit.Migrations
Old code ignores the new columns, so the schema change and backfill are safe to apply ahead of cutover. But an old web worker that creates or edits an evaluator after the backfill writes
paramsonly, leaving both FKs null — and after cutover those evaluators hit the "no LLM provider configured" error despite validparams, recoverable only by re-saving each one. Nothing re-syncs them automatically. Small window, small blast radius, but worth deciding deliberately: re-run the backfill after cutover, or hold this and makeparamsa read-through fallback for one release.The backfill deliberately leaves dangling ids null. Prod has exactly one such evaluator (
KMC - Coverage Eval, team Vaccine_Coach); it was already broken at run time and still needs a human to re-pick the model — the original id can't be recovered.Demo
Docs and Changelog