Conversation
90bf93d to
1c2600b
Compare
|
Rebased onto current What the rebase droppedThe two That also retires the "Depends on #342" note at the top of the description — #342 has merged. What the rebase preservedFour conflicts, each resolved by keeping both sides rather than taking one:
Three things needed updating because
The dash clock was a second consumer#380 added a Rather than keep the feature alive for it, the same technique this PR already uses for Lemonade is applied to dash. The principle: what must not diverge between the tested and the shipped binary is the compile. That is not the same as "no test affordances" — a path- or state-based affordance is present identically in every build and inert until something plants the state, whereas a So The existence check there is load-bearing rather than an optimisation: that field selects wall time only on Why serve-18 was failingNot infrastructure — a real defect in this PR. All three red lanes reported the same single unexpected failure, and the logs showed the planted runtime being ignored while a real multi-gigabyte backend install ran and succeeded, so Before With the install skipped, The planting location was already correct: this scenario does not opt into the shared runtime tree, so the CLI resolves no env root and Status of the three previously-red lanes
Also green on this head: Verified locally
Not verified locally: serve-18 itself — this host has no usable AMD GPU, which is precisely why the scenario is now |
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
I ran a code review pass on this in addition to my own read of the diff, and it surfaced two things worth looking at before merge.
The main one: dash_test_clock_offset_path in apps/rocm/src/dash.rs now checks for a file on every rocm dash launch, in every build, not just E2E builds. If that file (telemetry/test-clock-offset) ever ends up in a real user's data directory (synced or copied from a dev or CI machine, some future bug that touches that path), the daemon silently drops onto a frozen logical clock instead of wall time. There's no log line when this happens, only a doc comment saying it should never occur outside the E2E harness. A one line warn log when the file is found and used would make this fail loudly instead of silently, without touching the test seam design itself.
The second one is minor, and actually already flagged by the author in a comment: the clock file name and its data/telemetry subpath are duplicated as literals between dash.rs and the E2E's dash_steps.rs, kept in sync only by that comment. Not asking to block on this one, just noting it since the review turned it up.
CI is otherwise clean aside from the Strix Halo Windows and Ubuntu self hosted lanes, which I checked are flaky on main independent of this PR (3 of the last 4 completed main runs failed or were cancelled on those two lanes), so that's not a signal against this change.
| /// `TestClockDirective::FreeRunning(0)` logical clock. Returning the path | ||
| /// unconditionally would therefore take production off `Utc::now()`. | ||
| fn dash_test_clock_offset_path(paths: &AppPaths) -> Option<PathBuf> { | ||
| let path = paths.telemetry_state_dir().join(DASH_TEST_CLOCK_FILE); |
There was a problem hiding this comment.
Every build now takes this path, not just E2E builds. If telemetry/test-clock-offset ever exists in a real user's data dir, the dashboard silently freezes its telemetry clock instead of using wall time, with nothing logged. Worth a warn log when this file is found and used, so a stray file fails loudly instead of silently skewing displayed timestamps.
There was a problem hiding this comment.
Agreed, and fixed in 7841012 — this is the inherent cost of the state-based seam and it should not be paid silently.
dash_test_clock_offset_path now logs at WARN when it finds the file, naming the path:
WARN rocm::dash: dashboard telemetry clock is in TEST mode, not wall time: displayed
timestamps come from a logical clock read from this file. Only the E2E harness creates
it; delete it to restore wall-clock timestamps.
test_clock_file=/…/telemetry/test-clock-offset
Why tracing and not stderr. logging.rs makes the client subscriber file-only on purpose — the TUI owns the raw-mode terminal and a stray stdout/stderr write corrupts the display. The only production caller of this function is maybe_spawn_embedded_daemon on the rocm dash path, so stderr is exactly the wrong channel here; a line printed there would also land on the PTY grid that the dash-* scenarios parse and assert against. The WARN lands in ~/.rocm/logs/rocm-cli.log.<date>, which is where you would look when investigating "my timestamps are wrong" — and it survives the alt-screen switch, which a pre-launch eprintln would not.
Verified live, not just by unit test. Planting telemetry/test-clock-offset under a throwaway ROCM_CLI_DATA_DIR and launching the release rocm dash under a pty produced exactly the record above in the client log; the same launch with no file planted produced an empty log. logging::init runs at the top of run(), before subcommand dispatch, so the subscriber is installed before this callsite is reached, and DEFAULT_FILTER (warn,rocm=info,…) admits the rocm::dash target.
Tests. Two, using a thread-local capturing subscriber (tracing::subscriber::with_default, so a concurrently-running test cannot see or pollute the records): one asserts the WARN fires and names the file when it is present, one asserts nothing at all is logged when it is absent — a warning on every launch would be noise and would drain the signal out of the first.
Both were mutation-probed rather than just observed green: deleting the tracing::warn! turns the first test red (expected a WARN record, got: ), and moving the warn above the existence check so it fires unconditionally turns the second red. So neither passes vacuously.
The existence check itself is unchanged — cycle_timestamp picks wall time only on None, so returning Some for an absent file would put production on FreeRunning(0). Only the logging is new.
| /// `<ROCM_CLI_DATA_DIR>/telemetry/test-clock-offset` and falls back to wall time | ||
| /// when it is absent, so a rename on either side silently drops the whole | ||
| /// mechanism rather than failing loudly. | ||
| const DASH_CLOCK_OFFSET_FILE: &str = "test-clock-offset"; |
There was a problem hiding this comment.
This risk is already spelled out in the comment above (rename on either side silently breaks the mechanism). Not blocking, just flagging that the fix (a shared constant, or a debug_assert tying the two together) is still open.
There was a problem hiding this comment.
Closed this one too, with the debug_assert-style option rather than the shared constant — reasoning, since you offered both:
A shared constant is not reachable here. apps/rocm is a binary crate, so the suite cannot import DASH_TEST_CLOCK_FILE at all; making it importable means hosting it in a library and adding a first-party edge from e2e-cucumber to that library. #422 landed xtask check-crate-edges on main while this branch sat, which deliberately makes every such edge a reviewed decision — spending one on a single string, to couple the black-box suite to a production crate it otherwise doesn't link, seemed like the wrong trade.
So instead dash::tests::e2e_harness_plants_the_file_rocm_dash_reads in apps/rocm/src/dash.rs reads the step file's source with include_str! and pins the literals to what rocm dash actually resolves. That is the same cross-tree reach the suite already makes (installer_fixture.rs does it for install.ps1), it adds no dependency, and it runs in the every-PR unit lane rather than only where the dash scenarios run.
It checks two things:
- the harness's
DASH_CLOCK_OFFSET_FILEdeclaration parsed out of the source, compared againstDASH_TEST_CLOCK_FILE— parsed, not substring-matched, so an unrelated constant containing the same text can't satisfy it; - every directory between the data root and the file, derived from
AppPaths::telemetry_state_dir()and required to appear in order insidedash_clock_path's own body. Anchoring on the builder rather than the whole file matters: a stray.join("telemetry")elsewhere in those ~1.2k lines would otherwise satisfy it. Taking the whole relative path rather than its last component matters too —telemetry_state_dir()being moved (say todata/dash/telemetry) leaves the leaf name intact, and an earlier version of this test passed under that mutation while the mechanism was broken.
Mutation-probed in all four directions: renaming the constant on the production side, renaming it on the harness side, relocating telemetry_state_dir(), and moving the .join out of the builder while leaving a decoy elsewhere in the file — each fails with a message naming the mismatch.
It does impose two constraints on that file, both written into the comment above the constant: keep the declaration on one line, and keep dash_clock_path building the path from .join(..) links. If either is broken the guard fails loudly with an explanatory message rather than passing silently, which is the failure mode I wanted.
One related thing the rebase surfaced, since it is the same gate: this PR's own e2e-cucumber -> rocm-deps edge (the pinned Lemonade version behind the planted runtime's env_id) was not in the allowlist, so check-crate-edges failed on the rebased branch even though the rebase itself was conflict-free. That entry is added in 7841012.
Delete the `e2e-test-hooks` cargo feature and every seam it gated. The
feature cost the repo a CI-wide asymmetry: every lane running the full
suite pre-built `rocm` with it, so the binary under test was not the
binary released. The Linux `ci.yml` lifecycle lane packaged and installed
a hook-carrying build through the real installer, and the contract tests
encoded the split rather than removing it -- one lane asserted hook-free,
two asserted hook-ful, and Linux was covered by neither.
The principle applied throughout: what must not diverge between the
tested and the shipped binary is the COMPILE. This is not "no test
affordances" -- a path- or state-based affordance is present identically
in every build and inert until something plants the state, whereas a
`#[cfg(feature)]` makes the tested binary a different binary. Both
consumers move to the former.
Lemonade
--------
`@id:serve-lemonade-preparation-recovery` needs Lemonade's llama.cpp
backend install to fail twice so the scenario can pin the retry count and
the terminal recovery guidance. It now gets that from a planted runtime
instead of a compiled-in branch: one fixture binary copied to
`lemonade`/`lemond` in the scenario's isolated runtime dir, plus a
hand-written install manifest.
The manifest's `env_id` is the load-bearing field. Before `serve` prepares
an engine it calls `Detect` and skips its own install only when the
reported id equals the `lemonade-embeddable-{LEMONADE_VERSION}` that
`managed_engine_runtime_id` builds. A placeholder there does not fail
loudly -- it silently lets `serve` run the real multi-gigabyte install and
succeed, which is the opposite of the scenario's premise. The id is
therefore derived from `rocm_deps::LEMONADE_VERSION` rather than copied,
so a version bump cannot reintroduce that fallthrough.
With the install skipped, `serve_http` resolves the planted runtime and
reaches `ensure_best_llamacpp_backend`, whose `backends install` runs the
fixture and fails where a genuinely broken install fails: at a non-zero
child process. `resolve_runtime` accepts the manifest because it checks
only that it parses and that `lemond` is a file -- no checksum, version or
signature is involved on this path, so nothing here weakens a
verification the product performs, and `rocm` stays byte-identical to
what ships.
Seam #2 waived serve's no-GPU pre-flight so the scenario could reach the
install phase on a GPU-less host. Without it the pre-flight is reached
first, so the scenario is retagged `@requires-gpu`. That is the real cost
of this change: it moves off the blocking every-PR lane onto a gated one.
Tracked in the issue this closes.
Because the scenario no longer runs on a GPU-less lane, a unit test pins
the coupling that lane would otherwise silently lose: the fixture's
`backends` table must still parse into a `llamacpp:rocm` install attempt.
Dashboard clock
---------------
The logical observation clock added in #380 was the feature's other
consumer. The daemon side already ships unconditionally --
`RunnerOptions::test_clock_offset_path` is a plain field documented
test-only -- and only the CLI's env-var read was gated. `rocm dash` now
takes the directive file from `<ROCM_CLI_DATA_DIR>/telemetry/
test-clock-offset`, inside the data root it already resolves, so the
harness plants it exactly as it plants the Lemonade manifest.
The existence check there is load-bearing rather than an optimisation:
`test_clock_offset_path` selects `Utc::now()` only on `None`, and a
`Some(path)` whose file is absent leaves the daemon on its default
`FreeRunning(0)` logical clock. Returning the path unconditionally would
take production off wall time.
Workflows
---------
The two prebuilt-lane contract tests are repurposed rather than deleted.
"Match the feature xtask uses" becomes "match the invocation xtask uses",
which is the invariant that actually prevents the duplicate release
build, asserted whole-line so a trailing flag cannot satisfy it.
Closes #349.
Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
The logical-clock seam is state-based, so `dash_test_clock_offset_path` runs in every build, not only under the E2E harness. A stray copy of `telemetry/test-clock-offset` in a real data dir — synced from a dev or CI machine, or left behind by some future bug — therefore moves the dashboard off wall time with nothing to show for it, and the user has no way to explain the timestamps they are shown. Log at WARN when the file is found, naming it. It goes to `tracing` rather than stderr: the only caller runs on the `rocm dash` path where the TUI owns the terminal (see `logging.rs`), and a stray write there would corrupt the display — and would land on the PTY grid the dash scenarios assert against. Also pin the harness's copy of the file name and its `telemetry` subdirectory to production's. `apps/rocm` is a binary crate, so the suite cannot import the constant, and hosting it in a shared library would add a first-party crate edge for one string; instead a unit test reads the step file's source and fails in the every-PR lane if either side is renamed, which previously would have quietly dropped the whole mechanism. Declare the suite's `rocm-deps` dependency in the crate-edges allowlist that landed on main while this branch sat. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
1c2600b to
7841012
Compare
|
Thanks — both findings were right, and the first one is the real cost of choosing a state-based seam over a compile-time one, so it shouldn't be paid silently. Pushed as 7841012, on top of a rebase onto current The warn log. The duplicated literals. Took the Rebase. Onto Ran locally on this head: Also closed out one loose end from the previous description: the three Leaving both threads open for you to close. |
CI status correction — two things changed after this review was writtenFlagging this because the review reasonably discounted the self-hosted lanes, and both facts behind that have since moved. Neither was wrong at the time. 1. 2. That lane has since gone green on
The first completed result landed about three hours after this review. So the flakiness read was sound at the time and simply predates the evidence. The Ubuntu lane is a different story and the original read still holds there: it fails at GPU preflight under contention, is red on What that means for this PR
This was invisible until now because the Windows lane was cancelled on the previous head, so this PR has never had Windows data for the planted-runtime rework. Working hypothesis, not yet confirmed: #404 changed Not proposing to retag or weaken the scenario while the cause is unknown. |
Follow-up: my #404 hypothesis was wrong, and the real cause is a pre-existing Windows defectCorrecting the guess I posted above before it misleads anyone: #404 is not involved. It touched The real mechanism
After the spawn the parent waits up to 45s for HTTP readiness, records the status, and returns This is a real user-visible defect on Why this PR surfaced itThe deleted The planted runtime deliberately moves the failure to the fake's Why there is no test-side fixBoth seam-side options are closed, checked rather than assumed:
What I am not doingI am not retagging or weakening serve-18 while the cause is understood but unfixed, and I am not bolting a production Windows fix onto a test-infrastructure PR — that change deserves its own PR, its own tests, and a maintainer with a Windows host. Flagging it here for a maintainer decision rather than picking one unilaterally. Happy to open a separate issue for the Observed vs inferred, stated plainly: every file/line and git fact above is observed. That the fake's |
|
🔴 Automated review · pr-review-watcher · 7841012 This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer. SummaryDeletes the e2e-test-hooks cargo feature of the rocm crate and both seams it gated, replacing them with state-based affordances: the Lemonade recovery scenario now drives a planted fake runtime whose 🚫 Blocking (must fix before merge)None. Non-blocking
|
siloteemu
left a comment
siloteemu
left a comment
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 7841012
This is a formal review recording our position on the record. It is deliberately non-gating: this automation files no approval, so no approving review will appear here whatever the outcome, and the merge decision stays with a human reviewer.
Blocking: 0 · Non-blocking: 6. This round reviewed removal of a compile-time test seam in favour of state-based affordances, with the new cross-file guard shown to be non-vacuous by two mutation experiments run in a scratch copy.
Check conclusions this review worked from, read at this exact head immediately before publishing: 26 success, 1 skipped, 2 failure.
The full findings, including the non-blocking items, are in the review comment posted alongside this one.
Splitting this up — it has three blockers welded togetherRebasing this onto current What the rebase foundThe rebase itself was nearly clean — one additive conflict in #421 landed two new consumers of the feature this PR deletes, in the newest commit on
That is the same thing that happened with #380's dash clock while this branch sat. Two consumers added in the time this has been open — the removal is chasing a target that keeps moving, which is itself a signal. The three blockers
Bundled, they all inherit the worst one. What I have done
What happens to this oneParked as draft, not closed — the planted-runtime work is sound and I would rather re-land it than rewrite it. It needs #467 fixed first, because the scenario legitimately asserts behaviour Windows does not currently implement, and the Windows lane is now a required check. The feature removal should come last, as a small mechanical PR converting the remaining consumers in one go, so it lands before I have not pushed the rebase here — it does not compile for the reason above, and a broken head would be worse than a stale one. It is kept locally if it is useful later. Nothing here needs a decision from a reviewer right now; #466 is the piece that is ready. |
3 participants
Closes #349.
Problem
e2e-test-hooksis a cargo feature that compiles test-only seams intorocm. When #349 was filed it served exactly one scenario, but that is no longer true — #380 added a second, unrelated consumer after the issue was written:engines/lemonade/src/lib.rs— makes the llama.cpp backend install fail on demandapps/rocm/src/main.rs— waivesrocm serve's no-GPU pre-flight, so@id:serve-lemonade-preparation-recovery(serve-18) can reach the install phase on a GPU-less hostapps/rocm/src/dash.rs— gates the daemon's logical observation clock, whichdash-08anddash-09need to cross validity boundaries deterministicallyThe cost is spread across the repo: 2 feature declarations, 4
cfgsites, 10--featureslines acrossnightly.ymlande2e-selfhosted.yml,xtask's own build, and 3 contract tests.Two consequences follow from the feature existing at all.
Binaries under test are not the binaries shipped. Every lane that pre-builds with the feature installs a binary that differs from a release build. #342 fixed this for the Windows lifecycle lane; it was still true on Linux, where the release build happens inside
cargo xtask e2e— with the feature — so the Linux lifecycle lane packaged and installed a hook-carrying binary through the real installer.The contract tests encoded the asymmetry instead of removing it. One test asserted the Windows lane must be hook-free, two asserted self-hosted and nightly must be hook-ful, and
ci.yml's Linux lane was asserted by neither — the same blind spot that hid the duplicate-build drift #342 fixed, one lane over.The principle applied
The goal is no compile-time divergence between the binary under test and the binary shipped. It is not "no test affordances at all". An affordance that is present identically in every build, and that activates only on state a user's machine never has, satisfies the goal. A
#[cfg(feature)]does not, because it makes the tested binary a different binary.Both seams are therefore relocated rather than removed outright.
Fix, part 1 — the Lemonade failure becomes a planted runtime
tests/e2e-cucumber/src/bin/fake-lemonade.rsis a fixture binary copied into an isolated runtime directory under bothlemonadeandlemond(it takes its role fromargv[0]), alongside a hand-written install manifest. It reproduces the three calls the engine makes:statusexits 0,backendsprints a table reportingllamacpp:rocmas installable, andbackends installexits non-zero.The CLI then fails where a genuinely broken install fails — at a non-zero child process — and
rocmstays byte-identical to what ships.A bin target of
e2e-cucumberrather than an xtask-built artifact, so the suite reaches it throughCARGO_BIN_EXE_fake-lemonadewith no path to keep in step. It has to be a real executable, not a script: the engine spawns it by exact path and WindowsCreateProcessneeds a PE image.The manifest records the runtime id the engine actually looks for (derived from
rocm_deps::LEMONADE_VERSION), not a sentinel.ensure_self_managed_engine_readyskips the install only whendetect.env_idequals that id, so a placeholder value silently defeats the whole mechanism — the scenario performs a real multi-gigabyte install, succeeds, and fails its assertion.Fix, part 2 — the dash clock becomes a planted file
dash_test_clock_offset_pathno longer reads an environment variable behind acfg. It returns a fixed path inside the telemetry state dir the CLI already computes, and only when that file exists:The harness plants the file under the scenario's isolated root; nothing in
rocmever creates it, so a user's machine never takes this path.The existence check is load-bearing, not an optimisation.
cycle_timestampselects wall time only onNone; aSome(path)whose file is absent leaves the daemon on its defaultFreeRunning(0)logical clock. Returning the path unconditionally would take production offUtc::now()— so this is deliberately not a plain ungating of the old variable.No change to the daemon, or to how any data root is resolved.
RunnerOptions::test_clock_offset_pathwas already an ungated field.Because that check runs in every build, not only under the harness, finding the file is logged at WARN naming it (review feedback). A stray copy in a real data dir then explains itself in
~/.rocm/logs/rocm-cli.log.<date>instead of silently skewing displayed timestamps. It goes throughtracingrather than stderr because the only caller sits on therocm dashpath, where the TUI owns the raw-mode terminal (logging.rs) and a stderr write would also land on the PTY grid the dash scenarios assert against.The harness's copy of the file name and the
telemetrysubdirectory are no longer kept in step by a comment.apps/rocmis a binary crate, so the suite cannot import the constant, and hosting it in a shared library would spend a first-party crate edge (xtask check-crate-edges) on one string. A unit test reads the step file's source instead and pins the file name plus the whole directory chain, anchored inside the harness's own path builder, so a rename or a relocation on either side fails in the every-PR unit lane rather than quietly dropping the mechanism.Why this doesn't weaken verification
resolve_runtimeaccepts the planted manifest because it checks only that the manifest parses and thatlemondis a file. No checksum, version, or signature is involved on this path — the SHA-pinned archive download lives inprepare_embeddable, which the serve path does not go through. Nothing here bypasses a check the product performs.Approaches ruled out are recorded in #349 (mocking the request — the boundary is a subprocess, not HTTP; prewarming — ephemeral on hosted lanes, already installed on prewarmed ones; dropping the network; mocking
verify_sha256— a fail-open seam inside supply-chain verification).The tradeoff, explicitly
The pre-flight seam was what let serve-18 run without a GPU. Without it,
rocm servereaches its GPU-required pre-flight first, so the scenario is retagged@requires-gpuand moves off the blocking every-PR lane onto a gated one.That is a real loss of coverage cadence and it is the reviewer's call, not a cleanup side effect. The counter-argument is that what it covered on the every-PR lane was a code path that only existed in test builds.
To limit what that move gives up, a unit test in
engines/lemonadepins the coupling a GPU-less lane would otherwise silently lose: the fixture'sbackendstable must still parse into allamacpp:rocminstall attempt. If a parser change stopped accepting it, the scenario would quietly stop reaching the failure it asserts and only a GPU lane would notice. That test runs everywhere.dash-08anddash-09keep their existing cadence — they are@requires-os:linuxand still run on the blocking every-PR lane.Contract tests
assert_prebuilt_e2e_lanes_enable_test_hooksis repurposed, not deleted: "match the featurextaskuses" becomes "match the invocationxtaskuses", which is the invariant that actually prevents the duplicate release build. It asserts whole-line, so a trailing--features …cannot satisfy it — verified by re-adding the flag and watching it fail.#342's
!contains("e2e-test-hooks")assertion is dropped; its reuse assertions stay.Test plan
Verified locally:
backendstable,status→ 0,backends install→ non-zero,lemondstays alive until killed.dash-08anddash-09pass (2 scenarios, 22 steps, 0 failures), and the full dash feature passes (16 scenarios, 102 steps, 0 failures).--features rocm/e2e-test-hooksto a lane makesnightly_prebuilt_e2e_lanes_match_xtask_buildfail.cargo check --workspace --all-targets, pluscargo check -p e2e-cucumber --test e2eexplicitly (thee2etarget istest = false, so--all-targetsskips it).--workspace --all-targetsand-p e2e-cucumber --test e2e) andcargo fmt --checkclean,cargo xtask manifest --checkandcargo xtask check-crate-edgesclean,python scripts/smoke_local.pyclean.rocm dashlaunched under a pty with the clock file planted writes the record to the client log naming the file; the same launch with no file planted writes nothing. The two unit tests covering it were mutation-probed (removing the warn, and making it unconditional, each turn one of them red), as was the literal-pinning test (four probes: rename on either side, relocation of the telemetry dir, and moving the.joinout of the harness's builder).Verified on CI:
Limits, stated plainly:
dash-08/dash-09flaky in the first place. The mock lane itself is green on this head.rocm-dash-tuiagent tests fail in the development sandbox withcannot create token cache dir: Permission denied. This is now established as pre-existing rather than merely assumed: the same three fail identically on a clean checkout oforigin/mainwith none of this PR's changes, and this diff touches no file undercrates/rocm-dash-tui.