Repository navigation
Entry-resolution tasks: name the protein, not the id - #6
Conversation
…ll its entries Every MVS task handed the model the PDB id. Real users don't: the first evaluator prompt to fail live was "I wanna see a structure of PDE5A", answered with 1UJ7 — an entry that does not exist. Probing found the quieter failure too: real ids for the wrong protein (a SARS-CoV-2 RBD for "nanobody bound to GFP"). - tasks/mvs_resolve/: six tasks (PDE5A, PCSK9, CFTR, GFP, myoglobin, lysozyme), each with `accepted_ids` = every PDB entry mapped to the protein's UniProt accession (RCSB search, recorded under provenance) and a minimal polymer-cartoon reference on one canonical entry. scripts/generate_resolve_tasks.py regenerates. - grade_mvs(accepted_refs=…): any accepted id in the prediction is folded onto the reference's; anything else (1UJ7) mismatches as before. Runner and escalation pass the task's set through. - Tests: accepted set sanity, any-accepted-entry == 1.0, the 1UJ7 case scores below it, multi-structure references are left alone. 22 pass. - ROADMAP: B4 resolution, B5 colour-scheme prompt drift, B6 structured outputs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 50 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe grader now accepts configured alternate structure IDs for single-ID references. The runner passes accepted IDs to grading and escalation. Six resolution tasks and a task-generation script are added. The roadmap also records plans for color-theme grading and structured-output evaluation. ChangesEntry Resolution
Roadmap Updates
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Runner as run_mvs_task
participant Grader as grade_mvs
participant Escalation as escalating_grade
Runner->>Grader: Pass reference, prediction, accepted_ids
Runner->>Escalation: Pass reference, prediction, accepted_ids
Escalation->>Grader: Grade with accepted_refs
Escalation->>Escalation: Adjust copied reference for rendering when IDs match accepted IDs
Merge Risk: ⚪ Minimal · up to The reviewed changes have no identified issue that needs resolution before merge. Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The reviewed changes alter scoring without allowing predictions to choose the accepted-ID policy or gain additional execution, network, or file authority. Task generation uses fixed destinations and local output paths. No material security risk was identified in these changes. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 39.13% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 23 functions across 5 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces entry-resolution tasks where prompts name a protein instead of a specific PDB ID, allowing grading against multiple accepted structure IDs mapped to a UniProt accession. It adds a generation script, several new resolve tasks, and updates the grading logic in molbench/mvs.py to fold accepted predicted IDs onto the reference's canonical ID. Feedback on the changes highlights potential IndexError crashes in _fold_accepted_refs when handling malformed trees with empty tuples, suggesting the addition of tuple length checks to ensure robustness.
| ref_ids = {seg[1][1] for path in ref_paths for seg in path | ||
| if seg[0] == "download" and isinstance(seg[1], tuple) and seg[1][0] == "ref"} |
There was a problem hiding this comment.
If the reference or predicted tree contains a download node with empty or malformed parameters, seg[1] can be an empty tuple (). Accessing seg[1][0] without verifying the length of the tuple will raise an IndexError, potentially crashing the entire evaluation run. Adding a length check ensures robustness against malformed trees.
| ref_ids = {seg[1][1] for path in ref_paths for seg in path | |
| if seg[0] == "download" and isinstance(seg[1], tuple) and seg[1][0] == "ref"} | |
| ref_ids = {seg[1][1] for path in ref_paths for seg in path | |
| if seg[0] == "download" and isinstance(seg[1], tuple) and len(seg[1]) == 2 and seg[1][0] == "ref"} |
| if seg[0] == "download" and isinstance(seg[1], tuple) and seg[1][0] == "ref" \ | ||
| and seg[1][1] in accepted: |
There was a problem hiding this comment.
If the predicted tree contains a download node with empty parameters, seg[1] will be (). Accessing seg[1][0] directly will raise an IndexError and crash the grading process. Adding a length check prevents this crash when evaluating malformed model outputs.
| if seg[0] == "download" and isinstance(seg[1], tuple) and seg[1][0] == "ref" \ | |
| and seg[1][1] in accepted: | |
| if seg[0] == "download" and isinstance(seg[1], tuple) and len(seg[1]) == 2 and seg[1][0] == "ref" \ | |
| and seg[1][1] in accepted: |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca9d864fa2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # --- entry resolution (tasks/mvs_resolve) ------------------------------------------ | ||
|
|
||
| def _resolve_tasks(): | ||
| return [t for t in load_tasks(["mvs"]) if t.get("source") == "resolve"] |
There was a problem hiding this comment.
The repository CI runs PYTHONPATH=. python tests/test_harness.py, but all four new resolution tests are defined after the existing if __name__ == "__main__" block and are never called by that block. Consequently, the CI command exits successfully without exercising the accepted-ID behavior or validating the generated task sets. Move the main block below these definitions and invoke the new tests, or change CI to run pytest.
Useful? React with 👍 / 👎.
…download node without params Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks both — fixed in 4312683:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 431268369a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "attribute": "rcsb_polymer_entity_container_identifiers." | ||
| "reference_sequence_identifiers.database_accession", | ||
| "operator": "exact_match", "value": accession}}, |
There was a problem hiding this comment.
For res-cftr, the prompt asks for the CFTR channel, but this accession-only query accepts every entry containing any P13569-mapped polymer. The generated set consequently includes entries such as 1XMI and 2BBO, which are isolated NBD1 domains rather than channel structures; grade_mvs then folds those IDs to the full-length 5UAK reference and awards a matching scene full credit. Add sequence-coverage/domain constraints or curate accepted IDs per prompt so fragment-only answers do not corrupt the resolution score.
Useful? React with 👍 / 👎.
| accepted_refs: set[str] | list[str] | None = None, | ||
| ) -> dict[str, Any]: | ||
| g = grade_mvs(reference_tree, predicted_tree) | ||
| g = grade_mvs(reference_tree, predicted_tree, accepted_refs=accepted_refs) |
There was a problem hiding this comment.
When --escalate is enabled and a prediction uses a noncanonical accepted ID but differs elsewhere in the tree (for example, an equivalent all selector instead of polymer), this call folds the ID only for tree scoring. The visual tiers still render the canonical reference structure against the alternate accepted structure, so structural differences can dominate the pixel comparison and the VLM is explicitly asked whether both images contain the same structure, preventing an otherwise equivalent visualization from being rescued. Render a reference rewritten to the prediction's accepted ID, or otherwise carry the accepted-entry equivalence through T1/T2.
Useful? React with 👍 / 👎.
…alation renders the accepted entry; CI runs every test - generate_resolve_tasks: optional per-task min coverage (entity-level RCSB query); res-cftr drops isolated-domain entries such as 1XMI/2BBO. - escalate: the visual tiers re-point the reference at the accepted entry the prediction loaded, as the tree tier already does. - test_harness: the __main__ block runs every test_* function, so CI (which runs this file directly) can no longer skip a newly added test. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Codex: both fixed in abf4225.
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abf4225170
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| task = { | ||
| "id": f"res-{slug}", | ||
| "category": "mvs", | ||
| "source": "resolve", |
There was a problem hiding this comment.
When the leaderboard is regenerated with these tasks, scripts/build_leaderboard.py::_regime_of recognizes only generated/curated and grounded, then classifies every other source as Clinical (SIFTS/ClinVar). Consequently, all six new source: "resolve" tasks will be included in the clinical counts, means, and difficulty-spread claims even though they do no clinical or SIFTS grounding. Add an explicit resolution regime (including it in the displayed regime lists) or deliberately map this source to an existing non-clinical regime.
Useful? React with 👍 / 👎.
… unknown sources fail loudly Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Codex: fixed in 782c7a1. Sources now map to regimes through an explicit table. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 782c7a12e2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| # How a task's `source` maps to a difficulty regime, ordered easy -> hard. | ||
| REGIMES = ["Translation", "Grounded (ligand/SS)", "Clinical (SIFTS/ClinVar)"] | ||
| REGIMES = ["Translation", "Grounded (ligand/SS)", "Clinical (SIFTS/ClinVar)", "Resolution (by name)"] |
There was a problem hiding this comment.
When the leaderboard is rebuilt, regime_table() iterates this entire list while its heading and description still present the rows as an easy-to-hard difficulty gradient and assert that later regimes separate models more. Appending Resolution here therefore tells readers that name resolution is harder than Clinical, even though the adjacent comment and Finding 03 explicitly treat only the first three regimes as the gradient. Use a separate list for displayed, non-ordered regimes or update the table copy so the new row is clearly orthogonal.
Useful? React with 👍 / 👎.
| ("gfp", "show green fluorescent protein", "P42212", "1ema", | ||
| "The starter chip's target; the model tends to be right here — a control."), |
There was a problem hiding this comment.
For the generic prompt show green fluorescent protein, the generator accepts only entries mapped to UniProt P42212 because entries_for() performs an exact accession match. A model can therefore select a valid GFP structure from another organism or accession and be graded as wrong even though the prompt provides no species or accession constraint. Specify Aequorea victoria/P42212 in the prompt or include the other GFP accessions that satisfy this wording.
Useful? React with 👍 / 👎.
…culty gradient; GFP prompt names the species Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 participant
Every MVS task hands the model the PDB id. Real users don't. The first evaluator prompt to fail on the preview site was "I wanna see a structure of PDE5A", answered with 1UJ7 — an entry that does not exist. Probing 12 named-protein prompts on Haiku found the quieter version too: 0 non-existent ids but 3 real ids for the wrong protein (a SARS-CoV-2 RBD for "nanobody bound to GFP", DR5 for PCSK9, a plant Toc75 domain for CFTR).
Changes
tasks/mvs_resolve/— six tasks (PDE5A, PCSK9, CFTR, GFP, sperm whale myoglobin, HEWL). Prompts are verbatim/near-verbatim from the evaluator corpus and the probe. Each carriesaccepted_ids: every PDB entry whose polymer maps to the protein's UniProt accession (RCSB search, 50–1398 entries, recorded underprovenance), plus a minimal polymer-cartoon reference on one canonical entry so the task measures resolution, not styling.scripts/generate_resolve_tasks.pyregenerates them.grade_mvs(…, accepted_refs=…)folds any accepted id in the prediction onto the reference's canonical id before matching. A made-up id (1UJ7) is left alone and mismatches the download segment as before. Runner andescalating_gradepass the task's set through. No change for tasks withoutaccepted_ids.existssub-metric to separate "made-up" from "wrong-but-real"), B5 (colour-scheme prompt drift from the chat-driver), B6 (structured outputs experiment).Corpus goes 84 → 90 tasks; the live leaderboard is unaffected until a re-run.
🤖 Generated with Claude Code
Summary by CodeRabbit