feat(eval): discover trials from job metadata#321
Open
manzuoni-astera wants to merge 3 commits into
Open
Conversation
manzuoni-astera
requested review from
a team,
DorisMai,
k-chrispens and
marcuscollins
as code owners
manzuoni-astera
had a problem deploying
to
gpu-testing
GitHub Actions
Error
manzuoni-astera
had a problem deploying
to
gpu-testing
GitHub Actions
Error
manzuoni-astera
had a problem deploying
to
gpu-testing
GitHub Actions
Error
Contributor
📝 WalkthroughWalkthrough
ChangesMetadata-backed trial discovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant scan_grid_search_results
participant trial_directory
participant load_job_metadata
participant Trial
scan_grid_search_results->>trial_directory: Find refined.cif and job_metadata.json
scan_grid_search_results->>load_job_metadata: Load trial metadata
load_job_metadata-->>scan_grid_search_results: Return object or None
scan_grid_search_results->>Trial: Construct metadata-derived trial
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements metadata-backed discovery of completed grid-search trials by loading job_metadata.json during result scanning, so evaluators can rely on recorded trial identity and input paths instead of reconstructing them from directory names (with a compatibility fallback for historical runs). This aligns with Issue #213’s goal of sourcing map/structure locations from run metadata.
Changes:
- Added
load_job_metadata()and updatedscan_grid_search_results()to preferjob_metadata.jsonvalues (supporting bothmodelandmodel_name) with path-based fallback. - Extended the
Trialdataclass to carryinput_structure_path,density_path, andresolution. - Added/updated tests to ensure metadata is preferred and
Trial.__dict__.copy()includes the new fields.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/sampleworks/eval/grid_search_eval_utils.py |
Load and consume job_metadata.json when discovering trials; keep path-parsing fallback. |
src/sampleworks/eval/eval_dataclasses.py |
Add optional input paths and resolution fields to Trial. |
tests/eval/test_grid_search_trial_metadata.py |
New tests asserting metadata is preferred and invalid metadata is ignored. |
tests/eval/test_eval_dataclasses.py |
Update expected Trial.__dict__.copy() keys to include new fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+193
to
+199
| protein_value = metadata.get("protein", path_protein) | ||
| protein = str(protein_value) if protein_value is not None else None | ||
| model_value = metadata.get("model_name", metadata.get("model", path_model)) | ||
| model = str(model_value) | ||
| method_value = metadata.get("method", method) | ||
| method = str(method_value) if method_value is not None else None | ||
| scaler = str(metadata.get("guidance_type", scaler_dir.name)) |
Comment on lines
+208
to
+210
| gd_steps_value = metadata.get("num_gd_steps", params["gd_steps"]) | ||
| gd_steps = int(str(gd_steps_value)) if gd_steps_value is not None else None | ||
| ensemble_size_value = metadata.get("ensemble_size", params["ensemble_size"]) |
| scaler=scaler_dir.name, | ||
| ensemble_size=int(params["ensemble_size"]), | ||
| scaler=scaler, | ||
| ensemble_size=int(str(ensemble_size_value)), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/sampleworks/eval/grid_search_eval_utils.py (1)
202-210: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAvoid casting numbers to
strbeforeintandfloatconversion.Casting parsed JSON numbers to strings before converting them to
intorfloatcan cause runtime exceptions (for example, ifgd_steps_valueis parsed as a JSON float like10.0,int(str(10.0))will evaluate toint("10.0")and raise aValueError). Usingint()orfloat()directly handles both integer and float inputs cleanly.
src/sampleworks/eval/grid_search_eval_utils.py#L202-L210: changefloat(str(guidance_weight_value))tofloat(guidance_weight_value)andint(str(gd_steps_value))toint(gd_steps_value).src/sampleworks/eval/grid_search_eval_utils.py#L229-L244: changeint(str(ensemble_size_value))toint(ensemble_size_value)andfloat(str(metadata["resolution"]))tofloat(metadata["resolution"]).🤖 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/sampleworks/eval/grid_search_eval_utils.py` around lines 202 - 210, Remove the unnecessary string conversions before numeric parsing in the metadata handling logic: update guidance_weight_value and gd_steps_value conversions in src/sampleworks/eval/grid_search_eval_utils.py lines 202-210, and ensemble_size_value plus metadata["resolution"] conversions in lines 229-244, to pass the values directly to float() or int().
🤖 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 `@src/sampleworks/eval/grid_search_eval_utils.py`:
- Around line 202-210: Remove the unnecessary string conversions before numeric
parsing in the metadata handling logic: update guidance_weight_value and
gd_steps_value conversions in src/sampleworks/eval/grid_search_eval_utils.py
lines 202-210, and ensemble_size_value plus metadata["resolution"] conversions
in lines 229-244, to pass the values directly to float() or int().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aee01236-c4b7-40c7-96af-a5b92b84029f
📒 Files selected for processing (4)
src/sampleworks/eval/eval_dataclasses.pysrc/sampleworks/eval/grid_search_eval_utils.pytests/eval/test_eval_dataclasses.pytests/eval/test_grid_search_trial_metadata.py
manzuoni-astera
requested review from
Abdelsalam-Abbas,
denis-zaitsev and
mag-astera
as code owners
manzuoni-astera
had a problem deploying
to
gpu-testing
GitHub Actions
Error
manzuoni-astera
had a problem deploying
to
gpu-testing
GitHub Actions
Error
manzuoni-astera
had a problem deploying
to
gpu-testing
GitHub Actions
Error
marcuscollins
requested changes
Jul 15, 2026
marcuscollins
left a comment
marcuscollins
left a comment
Collaborator
There was a problem hiding this comment.
There's one issue about the altloc_occupancies that we should chase down. Otherwise LGTM.
manzuoni-astera
requested a deployment
to
gpu-testing
GitHub Actions
Waiting
manzuoni-astera
requested a deployment
to
gpu-testing
GitHub Actions
Waiting
manzuoni-astera
requested a deployment
to
gpu-testing
GitHub Actions
Waiting
Contributor
Author
|
Addressed the requested changes in
Focused evaluation/metadata tests pass ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
job_metadata.jsonwhile discovering completed grid-search trialsaltloc_occupanciesin new job metadata filesTrialmodeland forthcomingmodel_namemetadata schemasImplementation work for #213. Other evaluators can now migrate to the same
Trialfields; the legacy input/config arguments remain available for selections and historical runs.Validation
pytest tests/eval/test_grid_search_trial_metadata.py tests/eval/test_eval_dataclasses.py tests/eval/test_rscc_grid_search_script.py tests/utils/test_guidance_script_utils.py -q(34 passed)ty checkpassesruff checkandruff format --checkon touched filesgit diff --check