perf(hermes): remove build-only image caches#7492
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
added
integration: hermes
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughHermes Dockerfiles now isolate and remove build-only npm, Electron, node-gyp, and sandbox caches, verify their absence from published images, and add provisioning, dependency-layer, integrity-lookup, and runtime smoke-test coverage. ChangesHermes cache hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 8abdc43 in the TypeScript / code-coverage/cliThe overall coverage in commit 8abdc43 in the Show a code coverage summary of the most impacted files.
Updated |
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: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/hermes-share-mount-deps.test.ts`:
- Around line 218-240: Update the test “keeps only root runtime dependencies
after building workspace UIs (`#7144`)” to create marker files under
ui-tui/node_modules and web/node_modules before running the install layer. After
execution, assert both workspace node_modules directories are absent while the
root node_modules directory remains, using the resulting filesystem layout as
the primary verification; retain only call assertions needed for command
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8739337a-9993-4b58-a505-8aef49e035e5
📒 Files selected for processing (4)
agents/hermes/Dockerfileagents/hermes/Dockerfile.basetest/hermes-dashboard-provisioning.test.tstest/hermes-share-mount-deps.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- agents/hermes/Dockerfile
- agents/hermes/Dockerfile.base
- test/hermes-dashboard-provisioning.test.ts
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Exact-head refresh: Local verification on the exact head:
The merge commit is signed, DCO-compliant, and GitHub Verified. Fresh exact-head CI is running; no further code changes are planned unless CI or review finds an actionable issue. |
|
Fix-forward: dispatched the documented manual |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Exact-head credentialed E2E evidence for
Using the merged #7493 timing semantics (
The child scorecard job itself is intentionally skipped for PR-correlated dispatches because they provide Matched local images remain This proves exact-head behavior and the image-size reduction. It does not yet prove a hosted-runner memory reduction: the alternate-checkout trust boundary disables runner-pressure sampling. The remaining acceptance step is a matched direct-main cohort after merge/publish/pin, using #7493 for queue/execution data and runner-pressure telemetry for memory. The PR is otherwise green and awaits independent human review. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Fresh exact-revision validation is green after synchronizing with current
The controlled exact-revision A/B showed:
The cache-preservation guards passed in both Hermes lanes. The synchronized self-hosted run also resolved the root sandbox base by pull in 29s/31s instead of the stale-branch false rebuild path (16m16s/18m27s). Memory caveat: alternate-checkout controller runs currently disable hosted runner telemetry, so these runs establish time/export improvement and correctness, not a measured peak-memory reduction. The automatic controller failure was authorization-only ( |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/sandbox-provisioning.test.ts (1)
1390-1403: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the dashboard cache paths distinct.
/root/.npm,/root/.cache/electron, and/root/.cache/node-gypall map to the samerootCachedirectory. Cleanup of one path can therefore remove the shared evidence and let the test pass even if another cache path is not cleaned up. UserootCache/npm,rootCache/electron, androotCache/node-gyp, then assert each redirected directory is absent.As per path instructions, tests should prefer observable outcomes through the public boundary rather than relying on a shared implementation detail.
🤖 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 `@test/sandbox-provisioning.test.ts` around lines 1390 - 1403, Update the cache path replacements in the test setup around dockerRunCommandBetween so npm, electron, and node-gyp map to distinct rootCache subdirectories: npm, electron, and node-gyp. Add assertions through the test’s public observable boundary that each redirected directory is absent after cleanup, rather than relying on shared rootCache state.Source: Path instructions
🧹 Nitpick comments (1)
test/hermes-final-image-layout.test.ts (1)
218-222: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAvoid formatting-sensitive source-text assertions for cache behavior.
These checks can reject equivalent Dockerfile formatting while still not proving that
/sandbox/.cacheis absent from the assembled image. Prefer an image/layout assertion, or normalize the extracted command and assert command semantics rather than trailing whitespace, ordering, and literal\formatting.As per path instructions, tests should prioritize behavioral confidence over implementation lock-in.
🤖 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 `@test/hermes-final-image-layout.test.ts` around lines 218 - 222, Replace the formatting-sensitive cache assertions in the hermes final-image layout test with a behavioral image/layout assertion that verifies /sandbox/.cache is absent from the assembled image. Avoid asserting exact Dockerfile command text, trailing whitespace, command ordering, or literal line-continuation formatting; keep only semantic validation of the cache cleanup behavior.Source: Path instructions
🤖 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.
Outside diff comments:
In `@test/sandbox-provisioning.test.ts`:
- Around line 1390-1403: Update the cache path replacements in the test setup
around dockerRunCommandBetween so npm, electron, and node-gyp map to distinct
rootCache subdirectories: npm, electron, and node-gyp. Add assertions through
the test’s public observable boundary that each redirected directory is absent
after cleanup, rather than relying on shared rootCache state.
---
Nitpick comments:
In `@test/hermes-final-image-layout.test.ts`:
- Around line 218-222: Replace the formatting-sensitive cache assertions in the
hermes final-image layout test with a behavioral image/layout assertion that
verifies /sandbox/.cache is absent from the assembled image. Avoid asserting
exact Dockerfile command text, trailing whitespace, command ordering, or literal
line-continuation formatting; keep only semantic validation of the cache cleanup
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fd445cd9-9d24-4d7d-91a5-a003bc83b971
📒 Files selected for processing (5)
agents/hermes/Dockerfileagents/hermes/Dockerfile.basetest/hermes-final-image-layout.test.tstest/hermes-share-mount-deps.test.tstest/sandbox-provisioning.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- agents/hermes/Dockerfile
- agents/hermes/Dockerfile.base
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
CodeRabbit follow-up for the current-head review:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Exact-head audit for
The documented manual controller 30168456161 dispatched child 30168471044, but |
5 participants
Summary
The Hermes base image retained npm downloads, Electron archives, node-gyp headers, and production dependencies for every UI workspace after its build artifacts were complete. This change removes those build-only contents in their creating layers while preserving the Hermes runtime, browser tooling, dashboard, TUI, WhatsApp, and Discord behavior.
The final Hermes image also committed Hermes doctor's disposable
/sandbox/.cacheinto the layer that generated configuration, even though a later layer deleted the visible path. The follow-up removes that cache in its creating layer and keeps a final absence guard.Against a matched baseline build at
2f6298ee165f4821f635be962fedff5e165701ee, the measurement-head PR image fell from 1,057,410,491 bytes to 433,015,966 bytes: 624,394,525 bytes smaller (59.05%). Rootnode_modulesfell from 649,783,827 bytes to 79,784,358 bytes (87.72%).Related Issue
Part of #7144. This PR does not close the issue because repeated hosted-runner timing and peak-memory acceptance evidence remains.
Changes
/root/.npm,/root/.cache/electron, and/root/.cache/node-gypin the dependency-install layer that creates them./sandbox/.cachein the same layer that creates it, and reject a surviving path or symlink during final-image sealing.node_modulesunavailable.Type of Change
Quality Gates
f3ad9ca9e3d1778c0fb7eaa6add0607a868269ccagainst base2f6298ee165f4821f635be962fedff5e165701ee, and for the follow-up diff at0c96c1ae4d489ec20b5c008ce38763791014b654. The main-sync commits68885c7855fc90d0fa53bfab4f0bee8eced73784and8abdc43656ab20444450698b46f327dc3de69588do not change the reviewed PR-owned diff, andd6b46853f0b44820bd1f43ac2368d84de3ff2027only strengthens cache-cleanup test coverage. Hosted x86, arm64, and selected E2E validation pass for the reviewed implementation head; the test-only follow-up passes its focused integration suite.Documentation Writer Review
no-docs-neededmain; the PR-owned diff is unchanged.DGX Station Hardware Evidence
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 unavailabletest/sandbox-provisioning.test.ts: 61/61 integration tests passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable; the change is covered by focused runtime/image tests, a full exact-head image build, andnpx prek run --from-ref origin/main --to-ref HEAD.npm run docsbuilds without warnings (doc changes only)Additional image evidence:
docker build -f agents/hermes/Dockerfile.base -t nemoclaw-hermes-7144:exact-head .passed at measurement headf3ad9ca9e3d1778c0fb7eaa6add0607a868269cc. The first attempt ended in an npm registryECONNRESET; one controlled retry reused the cached layers and completed.agent-browser0.26.0 CLI, dashboard assets, the self-contained TUI, separate WhatsApp dependencies, and cache absence.68885c7855fc90d0fa53bfab4f0bee8eced73784; the later change only strengthens a test fixture. Peak-memory sampling remains outstanding because alternate-checkout controller runs disable hosted runner telemetry.f3ad9ca9eproduced a 480,356,238-byte final image and follow-up0c96c1ae4produced 457,589,433 bytes: 22,766,805 bytes / 4.74% smaller overall. The PR-added portion above the base fell 48.09%, and the doctor layer fell from 155 MB to 78.6 MB./sandbox/.cacheabsent, and passed a Hermes v0.18.0 runtime smoke. This is local image-size evidence, not yet hosted-runner peak-memory proof.Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit