Repository navigation
Harden DFX artifact validation: bind to the current run - #1254
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMultiple DFX smoke tests under a2a3 and a5 tensormap_and_ringbuffer suites now capture a Changesa2a3 DFX run-scoped output validation
Estimated code review effort: 3 (Moderate) | ~25 minutes a5 DFX run-scoped output validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TestRun as test_run
participant Validator as validate_perf_artifact / _post_validate
participant FS as OutputsDir
TestRun->>TestRun: run_marker = time.time()
TestRun->>TestRun: super().test_run(...)
TestRun->>Validator: validate(..., since/run_marker=run_marker)
Validator->>FS: glob candidate output directories
FS-->>Validator: directory list
Validator->>Validator: filter dirs where st_mtime >= run_marker
Validator->>Validator: assert at least one match, select newest
Validator->>FS: assert expected artifact file exists in selected dir
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
|
close #1249 |
![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 updates several test suites across the a2a3 and a5 platforms to ensure that validation checks bind only to the output directories generated during the current test invocation, rather than stale directories from prior runs. This is achieved by capturing a pre-run timestamp marker using time.time() and filtering output directories based on their modification time. The review feedback highlights a potential source of test flakiness on filesystems with coarse modification time resolutions (e.g., 1 second) and suggests subtracting a 2-second buffer from the captured timestamp to prevent current-run directories from being incorrectly filtered out.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/st/a2a3/tensormap_and_ringbuffer/dfx/pmu/test_pmu.py (1)
32-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConstant name still implies "prefix" semantics.
The docstring now explicitly states column order isn't enforced (membership-only), but the constant is still named
_REQUIRED_HEADER_PREFIX. Consider renaming to_REQUIRED_HEADER_COLUMNS(or similar) to match the corrected semantics.🤖 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 `@tests/st/a2a3/tensormap_and_ringbuffer/dfx/pmu/test_pmu.py` at line 32, The constant name in test_pmu.py still suggests prefix ordering even though the logic now only checks required members. Rename `_REQUIRED_HEADER_PREFIX` to something like `_REQUIRED_HEADER_COLUMNS` and update any references in the PMU test helpers so the name matches the membership-only semantics.tests/st/a2a3/tensormap_and_ringbuffer/dfx/l2_swimlane/_swimlane_validate.py (1)
49-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect fix; consider extracting the duplicated dir-selection helper.
The
since-based filtering here is correct. However, the identicalmatches = [...]; assert matches; out_dir = max(...)block is now copy-pasted acrosstest_args_dump.py,test_dep_gen.py,test_dep_gen_chain.py,test_pmu.py,test_scope_stats.py, andtest_l2_swimlane_mixed.py's_validate_dump_func_ids. This PR had to touch every one of those copies to fix the same stale-directory bug class — a shared helper (e.g._select_current_run_dir(safe_label, since)inscene_test.py) would centralize this logic and prevent future drift.♻️ Sketch of a shared helper
def _select_current_run_dir(safe_label: str, since: float) -> Path: matches = [p for p in _outputs_dir().glob(f"{safe_label}_*") if p.stat().st_mtime >= since] assert matches, f"no output dir for {safe_label!r} created this run" return max(matches, key=lambda p: p.stat().st_mtime)Also applies to: 104-104, 145-145
🤖 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 `@tests/st/a2a3/tensormap_and_ringbuffer/dfx/l2_swimlane/_swimlane_validate.py` around lines 49 - 70, The current since-based directory selection in validate_perf_artifact is correct, but the same matches/assert/max pattern is duplicated across several test validators and should be centralized. Extract the run-dir selection logic into a shared helper such as _select_current_run_dir(safe_label, since) in the common test utilities (for example scene_test.py), then update validate_perf_artifact and the other duplicated call sites to use it so stale-directory handling stays consistent and future fixes only need to land once.
🤖 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
`@tests/st/a2a3/tensormap_and_ringbuffer/dfx/l2_swimlane/_swimlane_validate.py`:
- Around line 49-70: The current since-based directory selection in
validate_perf_artifact is correct, but the same matches/assert/max pattern is
duplicated across several test validators and should be centralized. Extract the
run-dir selection logic into a shared helper such as
_select_current_run_dir(safe_label, since) in the common test utilities (for
example scene_test.py), then update validate_perf_artifact and the other
duplicated call sites to use it so stale-directory handling stays consistent and
future fixes only need to land once.
In `@tests/st/a2a3/tensormap_and_ringbuffer/dfx/pmu/test_pmu.py`:
- Line 32: The constant name in test_pmu.py still suggests prefix ordering even
though the logic now only checks required members. Rename
`_REQUIRED_HEADER_PREFIX` to something like `_REQUIRED_HEADER_COLUMNS` and
update any references in the PMU test helpers so the name matches the
membership-only semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c59a1d1e-7272-4de4-af83-15739ad5db2c
📒 Files selected for processing (16)
tests/st/a2a3/tensormap_and_ringbuffer/dfx/args_dump/test_args_dump.pytests/st/a2a3/tensormap_and_ringbuffer/dfx/dep_gen/test_dep_gen.pytests/st/a2a3/tensormap_and_ringbuffer/dfx/dep_gen/test_dep_gen_chain.pytests/st/a2a3/tensormap_and_ringbuffer/dfx/l2_swimlane/_swimlane_validate.pytests/st/a2a3/tensormap_and_ringbuffer/dfx/l2_swimlane/test_l2_swimlane.pytests/st/a2a3/tensormap_and_ringbuffer/dfx/l2_swimlane/test_l2_swimlane_mixed.pytests/st/a2a3/tensormap_and_ringbuffer/dfx/pmu/test_pmu.pytests/st/a2a3/tensormap_and_ringbuffer/dfx/scope_stats/test_scope_stats.pytests/st/a5/tensormap_and_ringbuffer/dfx/args_dump/test_args_dump.pytests/st/a5/tensormap_and_ringbuffer/dfx/dep_gen/test_dep_gen.pytests/st/a5/tensormap_and_ringbuffer/dfx/dep_gen/test_dep_gen_chain.pytests/st/a5/tensormap_and_ringbuffer/dfx/l2_swimlane/_swimlane_validate.pytests/st/a5/tensormap_and_ringbuffer/dfx/l2_swimlane/test_l2_swimlane.pytests/st/a5/tensormap_and_ringbuffer/dfx/l2_swimlane/test_l2_swimlane_mixed.pytests/st/a5/tensormap_and_ringbuffer/dfx/pmu/test_pmu.pytests/st/a5/tensormap_and_ringbuffer/dfx/scope_stats/test_scope_stats.py
0b0f30f to
23534b0
Compare
The DFX scene tests locate their output directory by globbing outputs/<label>_* and taking the newest by mtime. That let a stale directory from a prior run/session with the same label satisfy the glob and be validated in place of a missing current-run output, masking a capture regression; two helpers also silently returned when the glob matched nothing, so a total capture failure passed as green. Capture a whole-second time marker before each run (floored via int(time.time()) so a coarse-mtime filesystem that truncates directory mtimes to the second can't spuriously exclude the current run) and keep only directories created past it, then assert at least one exists. Applied in lockstep across the a2a3 and a5 mirrors: pmu, dep_gen, dep_gen_chain, args_dump, scope_stats, l2_swimlane (+ mixed) and the shared _swimlane_validate helper (now takes a since= marker). The PMU header check asserts an ordered prefix, not membership: the CSV header literal and the positional row writer in pmu_collector.cpp are separate code paths, so a header reordered out of sync with the rows would silently mislabel every data column for any consumer. The ordered assertion is what keeps the header in the row order.
23534b0 to
e5f0a2b
Compare
2 participants
What
The DFX scene tests locate their output directory by globbing
outputs/<label>_*and taking the newest by mtime, without confirming itbelongs to the current test invocation. Two problems, both from issue #1249's
section A:
outputs/<label>_<old-ts>/from aprior run/session with the same case label satisfies the glob and gets
validated in place of (or instead of) the current run's real output —
masking a current-run capture regression.
_swimlane_validate.pyanddep_gen/test_dep_gen.pyreturned without asserting when the glob matched nothing, so a total capture
failure (no output dir produced) passed as green.
How
Capture a
time.time()marker beforesuper().test_run(...), thread it intothe validators, and keep only directories whose
mtime >= marker; thenassertat least one such directory exists. Fixed in lockstep across thea2a3 and a5 mirrors so they stay byte-identical modulo arch strings:
pmu,dep_gen,dep_gen_chain,args_dump,scope_stats,l2_swimlane(+mixed)_swimlane_validate.validate_perf_artifactnow takes asince=markerPMU header check (A.3)
Kept membership-only. Confirmed nothing in the repo reads
pmu.csvpositionally (the only consumer is the test itself), so enforcing column
order would add brittleness without catching a real break. Instead the
docstring is corrected to state column order is not enforced — resolving the
doc/impl mismatch without tightening behavior.
Testing
All six DFX smokes pass on
a2a3sim(silicon-agnostic) with their--enable-*flags. The marker-filter + assert paths are exercised by realcaptures.
Scope
Refs #1249. Section B (adding the DFX smoke steps to the
st-sim-a5CI job)is intentionally left out of this PR.