Repository navigation
fix(tools): detect host and title changes in interval digests - #10559
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the digest key set, output behavior, CLI compatibility, and regression tests. The six omitted summary identity fields are now retained from the first interval and compared between adjacent intervals; optional fields remain compatible with older records, and appearance/disappearance is treated as an identity transition. The JSON timing rows and Markdown table logic remain unchanged. I ran the PR-head trace-digest suite: all 60 tests passed. Hosted eligibility, conventions, intake, labels, relinker matrix, and ASAN pass; full platform builds were skipped by eligibility. I found no issue in this change.
THEMANnt
left a comment
There was a problem hiding this comment.
Reviewed PR #10559 at head 8ee49632b059b7c98e57a0f9155f133f0c85bc85 against pinned base 672c38f70355fa46d3533c13bf404409374fa999. No actionable findings in this focused review.
digest_intervals() now retains and compares cpu, cpu_threads, memory_mib, title_id, title and content_version, matching fields emitted by the C++ summary serializer. Adjacent comparisons detect changes and returns to earlier values. Missing metadata remains accepted; zero and empty-string values remain distinguishable from absence. I inspected field-by-field regressions, removal/restoration, stable legacy metadata, existing commit/resolution changes, interval filtering and CLI output. The JSON identity and Markdown warning changes agree with the documentation; timing-row construction remains unchanged.
Controller-recorded local runs of tools/tests/test_trace_digest.py on Windows AMD64 with Python 3.11.9 passed 55 baseline tests, 60 pristine-PR tests and 60 candidate tests. Running candidate tests against baseline production produced 15 assertion failures, including subtests: missing retained metadata and missed transitions demonstrate the old defect; two failures concern only updated warning wording. These were assertion failures, not setup failures. The repository conventions check also passed.
GitHub-reported effective checks succeeded for relinker-asan, relinker (ubuntu-latest, g++), relinker (windows-latest, g++), both relinker (macos-latest, clang++) variants (including x86_64), eligibility, conventions, labels and intake. Checks named linux, windows, register-context and write-watch were skipped. The snapshot reports no superseded executions or current failures; skipped builds are not passing product tests.
The author's additional fixture, CTest and compiler claims remain separate from independent local evidence. No game, GPU, console-hardware or full application validation supports this approval.
roboterspinne
left a comment
There was a problem hiding this comment.
Reviewed head 8ee4963 merged onto upstream/main befe264 (2026-10-24); the merge is clean, check_conventions.py passes and pr_overlap.py lists only three base-conflicted PRs it could not compare (#674, #1835, #5021), all sharing docs/dev/BUILD.md only.
Environment: Windows 11 Home 10.0.26200, Python 3.13.9 (Anaconda); Python only, no build needed.
Results
python -m unittest tools.tests.test_trace_digeston the merged tree: 60 tests pass in 12 s. Red/green: againstmain'strace_digest.pyfifteen subcases fail across the four new tests and the reworded existing one; all pass on the branch.- The six new identity keys are exactly the fields the recorder writes (
PerformanceTrace.hpp:704-712) and the optional fieldsperformance_compare.pyalready compares (OPTIONAL_STRINGS,OPTIONAL_INTEGERS), so the three tools name them alike. - Absent and empty count as different (
Noneagainst""or0), the rule the seven old keys already follow. That fits the recorder, which writescpu,cpu_threadsandmemory_mibtogether only when the host is known and the three title fields together only when a title is set, so a key appearing or disappearing is a real launch boundary, not noise. - #10367 (
fix(tools): validate trace summary JSONL records, opened 2026-10-23) edits the same two files; its hunks (trace_digest.pyat lines 93 and 284, the test file at line 46) are disjoint from yours (308 and 322, 652) and a three-way merge of the two heads is clean, so no conflict, but the body's overlap paragraph does not mention it.
Nit
trace_digest.py:323: the Markdown header still prints only GPU, driver, OS, resolution and commit, while the warning three lines down now says "host ... or title metadata changed". A reader of a pasted digest therefore cannot see which CPU, title or content version the trace starts with, or what changed; only--jsonhas it. Printingtitle_id,title,content_versionandcpuwhen present would close that (envwas already missing from the header onmain). The body limits the Markdown change to the warning, so if that is deliberate, a follow-up.
Notes, nothing to change here: test_resolution_changes_are_reported (line 692) pins behaviour main already had, since resolution was in the key tuple; it fails on main only because of the new wording, which now names resolution where the old message did not. Fine as a regression guard, just not one of the metadata regressions the body counts. The CLI test compares the subprocess output with digest_intervals() of the same lines minus the six keys, both from this branch, so it pins JSON round-tripping and identity_changed_at == [1] through the real entry point rather than a difference to main; fine.
Not checked: Linux and macOS runs, a real aps5-summary.jsonl from a title.
I found no blocking issue: the key set is right, the comparison rule is unchanged, timing rows never read the new keys, and the counter-check holds. For disclosure: I have open PRs (#10667 on tools/tests/test_suggest_patches.py, and relinker PRs #10679, #10658, #10646, #10607, #10596, #10564, #10403, #10402, #10401, #10391, #10390); none touches this tool.
Written with AI assistance (Claude Code). I ran the tests above on my Windows 11 machine.
| self.assertEqual(result["identity_changed_at"], []) | ||
| self.assertNotIn("changed at interval", trace_digest.markdown_intervals(result)) | ||
|
|
||
| def test_resolution_changes_are_reported(self): |
There was a problem hiding this comment.
Note, nothing to change: resolution was already in the key tuple on main, so this pins existing detection and fails on main only because of the new wording (which now names resolution, the old message did not). Fine as a regression guard, just not one of the metadata regressions the body counts.
| @@ -322,7 +323,7 @@ def markdown_intervals(result): | |||
| f"{identity['gpu']}, {identity['driver']}, {identity['os']}, {identity['resolution']}, commit {identity['commit']}" | |||
There was a problem hiding this comment.
Nit: the header still prints only GPU, driver, OS, resolution and commit, while the warning three lines down now says "host ... or title metadata changed", so a reader of a pasted digest cannot see which CPU, title or content version the trace starts with, or what changed; only --json has it. Printing title_id, title, content_version and cpu when present would close that (env was already missing here on main). If limiting the Markdown change to the warning is deliberate, a follow-up.
What
APS5_TRACE_SUMMARY_EVERYappends completed intervals toaps5-summary.jsonl, including across launches. The interval digest ignores six identity fields already emitted byTraceFormat::Summary:cpu,cpu_threads,memory_mib,title_id,titleandcontent_version. A file containing intervals from different titles, versions or hosts can therefore produce no identity-change warning, and JSON output loses those fields.Retain the first interval's CPU/title metadata in the JSON identity and compare it between adjacent intervals alongside the existing identity fields. Update the Markdown warning and the existing build-guide description. Logs without optional metadata remain accepted; timing rows, tables and sparklines keep their behavior.
Tested
Windows, Python 3.12.14, CMake 4.3.1, WinLibs GCC 15.2.0; baseline main
7be57380e35b0faddf393b69f1633da73ff51080:TraceFormat::Summaryserializer to emit ten synthetic interval records with nine actual identity transitions. Main detects only the GPU control transition and omits all six CPU/title fields; this change retains those fields and detects all nine transitions.python -I tools/tests/test_trace_digest.py IntervalDigestTests -v: 18/18 passed after the fix.ctest --test-dir build-relinker -R '^(trace_digest|test_script_registration)$' --output-on-failure --timeout 30: 2/2 passed. The trace suite ran all 60 tests without skips; CTest completed it in 18.61 seconds. All 176 Python test scripts are registered.python tools/check_conventions.py --base origin/mainandgit diff --check origin/main...HEADpass.tools/pr_overlap.py --branch fix/trace-digest-interval-identity: no confirmed overlap against 1,264 open PRs. Four comparisons were blocked by existing base conflicts (feat(input): add native SDL input configuration with ImGui #674, feat(aot): discuss native ARM64 compiler architecture and integration #1835, feat(aot): consolidate native ARM64 compiler, runtime and consumers #5021 and feat(agc): trace device memory allocations and cache budgets #10194); exact-head manual review confirms none changes the trace tool/tests or our interval-digest documentation sentence. The repository checker ran unchanged through a local adapter supplying public GitHub pagination becauseghis unavailable.No application performance improvement is claimed. No title, console hardware, GPU or full system-library build was tested; this PR changes only the Python report, its tests and existing documentation.
Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)