fix(evaluator): load globbed dataset files in a deterministic order - #1164
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughDataset file discovery now sorts recursive and glob-matched paths. Tests verify deterministic directory and glob ordering. The evaluator plugin test updates expected scores for sorted file-path order. ChangesDeterministic dataset ordering
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
`discover_files` returned whatever order `rglob`/`glob` got back from the filesystem. Rows from those files are concatenated in that order and their positions become `row_index` on the resulting scores, so the same fileset scored twice could pair a row's score with a different input row. This is how `test_fileset_fragment_and_glob_datasets` was failing intermittently: globbing `part-*.json` over two files yielded either `[1.0, 0.0, 1.0]` (part-a first) or `[1.0, 1.0, 0.0]` (part-b first). The test had pinned the latter, so it passed or failed on readdir order. Sort both branches, matching what `agent_seeds.py`, `harbor_runtime.py`, and `fabric/skills.py` already do for their directory walks. The e2e expectation moves to sorted order, and two unit tests pin the guarantee directly -- the existing glob test wraps both sides in `sorted()`, so it could not have caught this. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
deterministic-dataset-file-order/schapman
branch
from
August 7, 2026 13:48
3a48a10 to
a3e60b8
Compare
Contributor
|
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.
Why
discover_filesreturned whatever order the filesystem'sreaddirhanded back:Rows from those files are concatenated in that order, and their positions become
row_indexon the resulting scores. So the same fileset scored twice could pair a row's score with a different input row — anyone correlating scores back to their inputs gets silently wrong pairings.parallelism=1doesn't help; this happens before scoring.How it surfaced
e2e/test_evaluator_plugin.py::test_fileset_fragment_and_glob_datasetswas failing intermittently inKind CPU e2e:Same multiset, different order. Globbing
part-*.jsonover two files gives:[1.0, 0.0, 1.0][1.0, 1.0, 0.0]The test had pinned the second — the arbitrary order it happened to observe — so it passed or failed on readdir order. The test was the symptom; the loader was the bug.
After
Both branches sort. This follows what the codebase already does elsewhere for directory walks —
agent_seeds.py:66,harbor_runtime.py:501,fabric/skills.py:492— rather than introducing a new convention.The docstring records why, since "sorted" reads as cosmetic tidiness unless you know
row_indexdepends on it.TestDiscoverFilespin the guarantee, one per branch, with files created back-to-front so a filesystem returning creation order fails them. Worth noting the neighbouringtest_glob_pattern_discovers_fileswraps both sides insorted()— it deliberately avoided asserting order, so it could not have caught this.Verification
ruffclean,tyat themainbaseline (9 warning-level diagnostics, unchanged).packages/nemo_evaluator_sdk/tests: 1440 passed.Split out of #1069, which is where the flake was diagnosed; this is an independent behavior fix and doesn't depend on it.
Summary by CodeRabbit
Bug Fixes
Tests