fix: map Harbor trial folders to eval case ids - #104
Conversation
--refine keyed trajectories by the raw trial directory name, but Harbor writes folders like case-001__Lmi47iy. Split off the suffix so trajectories.get(case id) actually finds them. Fixes NVIDIA#93 Signed-off-by: mimran-khan <mohammed_imran.khan@outlook.com>
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
Reviewed the exact current head against #93 and the Harbor persisted-trial naming path. Suffixed folders map back to case ids and legacy unsuffixed folders remain unchanged. Focused tests pass (18), with Ruff and diff checks clean. No actionable findings; approved. The red Gitleaks job is unrelated branch history.
|
@mimran-khan - Please resolve merge conflicts |
|
Merged main in. Changelog conflict is resolved. |
# Conflicts: # CHANGELOG.md
|
Merged latest main (including #102) and resolved the CHANGELOG conflict. Ready for another look. |
|
Please resolve merge conflict in CHANGELOG.md so that I can merge it. Thanks for your contribution and patience @mimran-khan |
Head branch was pushed to by a user without write access
|
|
||
| def _case_id_from_trial_dir(trial_dir_name: str) -> str: | ||
| """Map Harbor trial folders like ``case-001__Lmi47iy`` back to the eval case id.""" | ||
| return trial_dir_name.split("__", 1)[0] |
There was a problem hiding this comment.
[P2] Resolve case IDs from persisted reward metadata
I took another final review pass and found one remaining issue that should be fixed before merge. Splitting at the first __ only handles simple <case-id>__<suffix> folders. Stop-on-pass aggregation persists <job-name>__<child-trial>, valid case IDs may themselves contain __, and folder names can differ from the canonical ID. A fresh reproduction mapped a stop-on-pass trial to the job prefix and collapsed case__one and case__two into the same case key. Each persisted trial already carries the canonical reward.json.entry_id. Can we prefer that value, keep folder parsing only as an unambiguous legacy fallback, and add coverage for stop-on-pass trials, __ in IDs, long or truncated IDs, missing metadata, and collisions before we merge?
There was a problem hiding this comment.
Should be fixed at 5cb7de6. Case ids now come from reward.json / verifier/reward.json entry_id first. Folder parsing only runs when the __ suffix looks like a Harbor attempt or random tail; otherwise the full directory name is kept so case__one and case__two stay distinct. Added coverage for stop-on-pass folders, double-underscore ids, and missing metadata.
Merge main and prefer reward.json entry_id when mapping persisted trial folders to eval case ids. Use folder-name parsing only for unambiguous Harbor suffix tails; keep full folder names when metadata is missing.
|
Merged main and addressed the reward entry_id feedback at 5cb7de6. Ready for re-review. |
3 participants
Summary
--refinekeyed trajectories by the raw Harbor trial folder name. Collection writescase-001__Lmi47iy, and eval cases arecase-001, so every lookup missed.Discovery now splits off the
__suffixand keys by the case id. A folder namedcase-001still works. Fixes #93.Verification
make lintmake testmake buildRelease Impact
CHANGELOG.md