test(e2e): capture rebuild-hermes phase timings for perf comparison - #7149
Conversation
Signed-off-by: Atulya Singh <atulyarajsingh@gmail.com>
|
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 selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughHermes rebuild progress now records completed phases and total elapsed time, exposes timeline snapshots, normalizes timing data with runner metadata, and writes a versioned JSON artifact. New support tests cover timing, stopping, runner classification, and normalization. ChangesHermes rebuild timing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HermesE2ETest
participant RebuildHermesProgress
participant RunnerSampler
participant TimingSummary
participant Artifacts
HermesE2ETest->>RebuildHermesProgress: collect timeline
HermesE2ETest->>RunnerSampler: collect runner metadata
HermesE2ETest->>TimingSummary: build normalized summary
TimingSummary->>Artifacts: write rebuild-hermes-timing.json
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Co-authored-by: Atulya Singh <atulyarajsingh@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Atulya Singh <atulyarajsingh@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
left a comment
There was a problem hiding this comment.
Approved exact head 60a4b4b against base 980e348. Independent review found no correctness or security blockers across all four changed files. Focused e2e-support validation passed 47/47 tests; formatting, test-project membership, and test-title checks passed. All current CI, CodeQL, DCO/commit verification, and automated advisors are green with zero unresolved threads. Credentialed E2E is safely recorded as an approved fork skip bound to this exact head/base in run 29659578683.
Summary
Issue #7144 requires proving a median wall-time improvement of at least 25% for
the
rebuild-hermeslanes "on the same runner class across repeated runs"(acceptance criterion 4). That evidence needs a stable, machine-readable timing
record before any cache or fixture change lands. This PR adds that measurement
capture to both
rebuild-hermeslanes without changing any behavior under test.Each run now emits a
rebuild-hermes-timing.jsonartifact containing:normalorstale-base),memory) so before/after runs can be confirmed to share a runner class before
they are compared,
The existing progress reporter already timestamped every phase transition; this
extends it to also accumulate the durations it was already computing, and adds a
pure summary/serialization module so the artifact shape is unit-testable without
Docker or hosted inference.
Related Issue
Part of #7144 (incremental; does not close the issue). This lands the
measurement foundation for acceptance criterion 4 ("Before/after timings are
collected on the same runner class across repeated runs"). The cache and pinned-
fixture work the issue also tracks remains follow-up, and now has a baseline to
measure against.
Note for reviewers: the issue's "persist reusable BuildKit cache across jobs"
scope item conflicts with the existing
requireDockerEngineRebuildsboundary intools/e2e/workflow-boundary.mts, which intentionally keeps everyrebuild-*job on the Docker engine cache (no external buildx builder). That trade-off needs
a maintainer design decision and is deliberately out of scope here.
Changes
test/e2e/live/rebuild-hermes-timing.ts(new): puredescribeRunnerClassandbuildRebuildHermesTimingSummaryhelpers plus the timing/runner-class types.No I/O beyond an injectable
ossample; durations normalize to non-negativewhole milliseconds so repeated runs stay directly comparable.
test/e2e/live/rebuild-hermes-progress.ts: accumulate each completed phasespan and expose
timeline(), a non-mutating snapshot that closes thein-flight phase at read time and freezes at
stop(). Existing heartbeat/logoutput is unchanged.
test/e2e/live/rebuild-hermes.test.ts: write therebuild-hermes-timing.jsonartifact at the end of the run (before the final credential-scan gate, so the
timing survives an assertion failure). No phase, assertion, or command changes.
test/e2e/support/rebuild-hermes-timing.test.ts(new): unit coverage for thetimeline accumulation (completed/in-flight/post-stop) and the summary builder
(lane labeling, duration normalization, runner-class fingerprint fallback).
No new configuration, fallback, migration, or compatibility layer is introduced,
and no external buildx builder or cache backend is added, so the
requireDockerEngineRebuildsboundary is preserved.Type of Change
Quality Gates
instrumentation with no user-facing behavior change.
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project e2e-support test/e2e/support/rebuild-hermes-timing.test.ts test/e2e/support/rebuild-hermes-progress.test.ts test/e2e/support/e2e-workflow.test.ts→ 46 passed;npm run typecheck:cli→ clean;npm run test:projects:check→ membership exact. The liverebuild-hermeslane runs in the E2E workflow (Docker + hosted inference required, not runnable locally).npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: not applicable (additive E2E instrumentation for two live lanes).npm run docsbuilds without warnings (doc changes only)Signed-off-by: Atulya Singh atulyarajsingh@gmail.com
Summary by CodeRabbit
New Features
rebuild-hermes-timing.json) that capture lane, runner environment details, per-phase timings, and timestamps using a stable schema.Tests