fix: Move DYNAMO_COMMIT_SHA to not invalidate image layers

[nv-kmcgill53]

Overview:

• Fixes: https://github.com/ai-dynamo/dynamo/pull/4414/files#r2539229494
• Fixes DYNAMO_COMMIT_SHA invalidating the cache layers below it causing build performance degradation.

Details:

Where should the reviewer start?

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

• closes DIS-1117

Summary by CodeRabbit

• Chores
  • Optimized Docker build cache behavior by adjusting build stage ordering across multiple Dockerfile configurations
  • Improved test assertion reliability for build verification

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

All four Dockerfile variants (base, sglang, trtllm, vllm) have been modified to defer DYNAMO_COMMIT_SHA argument binding to later build stages to optimize caching behavior. Additionally, a test assertion in test_build_sh.py was refactored to verify substring presence in stdout.

Changes

Cohort / File(s)	Summary
Docker cache optimization container/Dockerfile, container/Dockerfile.sglang, container/Dockerfile.trtllm, container/Dockerfile.vllm	Relocated DYNAMO_COMMIT_SHA ARG and ENV declarations from early to late positions within runtime and/or dev stages to defer cache invalidation and preserve layer caching.
Test assertion refactoring tests/ci/test_build_sh.py	Changed assertion in test_build_with_git_url_and_commit to verify substring containment in stdout rather than direct string truthiness.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Repetitive, consistent pattern applied across four similar Dockerfile variants minimizes per-file review variance
• Test change is straightforward substring assertion logic
• Primary focus: verify timing of ARG/ENV placement doesn't introduce unintended layer dependency shifts or expose the SHA prematurely

Poem

🐰 Cache layers dance, we shuffle them round,
DYNAMO_COMMIT_SHA moves without a sound,
Late-bind the secret to dodge invalidation's cost,
Build times stay swift, no precious cache lost! ✨

Pre-merge checks

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: moving DYNAMO_COMMIT_SHA to prevent cache invalidation in Docker image layers.
Description check	✅ Passed	The description includes overview, details section (though minimal), reviewer guidance section, and related issue reference, but lacks substantive detail in the Details section.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/ci/test_build_sh.py (1)

269-287: Assertion on TRTLLM wheel output looks good; consider loosening the path check (optional).

Using in stdout rather than matching the whole output is a nice robustness improvement. The only minor nit is the hard-coded /tmp/trtllm_wheel/ path: if the default output directory ever changes, this test will need to be updated. If you don’t need to assert the exact path, you could instead assert on just the message prefix or parse the directory from stdout.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5d9fc85 and 4db6f85.

📒 Files selected for processing (5)

• container/Dockerfile (1 hunks)
• container/Dockerfile.sglang (2 hunks)
• container/Dockerfile.trtllm (1 hunks)
• container/Dockerfile.vllm (2 hunks)
• tests/ci/test_build_sh.py (1 hunks)

🧰 Additional context used

🧠 Learnings (11)

📓 Common learnings

Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 2797
File: container/Dockerfile:437-449
Timestamp: 2025-08-30T20:43:49.632Z
Learning: In the dynamo project's devcontainer setup, the team prioritizes consistency across framework-specific Dockerfiles (like container/Dockerfile, container/Dockerfile.vllm, etc.) by mirroring their structure, even when individual optimizations might be possible, to maintain uniformity in the development environment setup.

Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 3051
File: container/templates/Dockerfile.vllm.j2:221-233
Timestamp: 2025-09-16T17:16:07.820Z
Learning: In the dynamo project, when converting Dockerfiles to Jinja2 templates, the primary goal is backward compatibility - generated Dockerfiles must be identical to the originals. Security improvements or other enhancements that would change the generated output are out of scope for templating PRs and should be addressed separately.

Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 2797
File: .devcontainer/devcontainer.json:12-12
Timestamp: 2025-08-30T20:43:10.091Z
Learning: In the dynamo project, devcontainer.json files use templated container names (like "dynamo-vllm-devcontainer") that are automatically processed by the copy_devcontainer.sh script to generate framework-specific configurations with unique names, preventing container name collisions.

📚 Learning: 2025-08-30T20:43:49.632Z

Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 2797
File: container/Dockerfile:437-449
Timestamp: 2025-08-30T20:43:49.632Z
Learning: In the dynamo project's devcontainer setup, the team prioritizes consistency across framework-specific Dockerfiles (like container/Dockerfile, container/Dockerfile.vllm, etc.) by mirroring their structure, even when individual optimizations might be possible, to maintain uniformity in the development environment setup.

Applied to files:

• container/Dockerfile
• container/Dockerfile.vllm
• container/Dockerfile.sglang
• container/Dockerfile.trtllm

📚 Learning: 2025-09-16T17:16:07.820Z

Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 3051
File: container/templates/Dockerfile.vllm.j2:221-233
Timestamp: 2025-09-16T17:16:07.820Z
Learning: In the dynamo project, when converting Dockerfiles to Jinja2 templates, the primary goal is backward compatibility - generated Dockerfiles must be identical to the originals. Security improvements or other enhancements that would change the generated output are out of scope for templating PRs and should be addressed separately.

Applied to files:

• container/Dockerfile
• container/Dockerfile.vllm
• container/Dockerfile.sglang
• container/Dockerfile.trtllm

📚 Learning: 2025-08-30T20:43:10.091Z

Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 2797
File: .devcontainer/devcontainer.json:12-12
Timestamp: 2025-08-30T20:43:10.091Z
Learning: In the dynamo project, devcontainer.json files use templated container names (like "dynamo-vllm-devcontainer") that are automatically processed by the copy_devcontainer.sh script to generate framework-specific configurations with unique names, preventing container name collisions.

Applied to files:

• container/Dockerfile
• container/Dockerfile.vllm
• container/Dockerfile.sglang
• container/Dockerfile.trtllm

📚 Learning: 2025-08-30T20:43:10.091Z

Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 2797
File: .devcontainer/devcontainer.json:12-12
Timestamp: 2025-08-30T20:43:10.091Z
Learning: In the dynamo project's devcontainer setup, hard-coded container names in devcontainer.json files serve as templates that are automatically processed by the copy_devcontainer.sh script to generate framework-specific configurations with unique names, preventing container name collisions.

Applied to files:

• container/Dockerfile
• container/Dockerfile.vllm
• container/Dockerfile.sglang
• container/Dockerfile.trtllm

📚 Learning: 2025-08-15T02:01:01.238Z

Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 2453
File: deploy/dynamo_check.py:739-741
Timestamp: 2025-08-15T02:01:01.238Z
Learning: In the dynamo project, missing cargo should be treated as a fatal error in dynamo_check.py because developers need cargo to build the Rust components. The tool is designed to validate the complete development environment, not just import functionality.

Applied to files:

• container/Dockerfile

📚 Learning: 2025-10-17T22:57:12.526Z

Learnt from: oandreeva-nv
Repo: ai-dynamo/dynamo PR: 3700
File: lib/llm/src/block_manager.rs:19-19
Timestamp: 2025-10-17T22:57:12.526Z
Learning: The dynamo library (ai-dynamo/dynamo repository) is currently developed to run on Linux only, so Linux-specific code and syscalls do not require platform guards.

Applied to files:

• container/Dockerfile
• container/Dockerfile.vllm

📚 Learning: 2025-09-03T01:10:12.599Z

Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 2822
File: container/Dockerfile.vllm:343-352
Timestamp: 2025-09-03T01:10:12.599Z
Learning: In the dynamo project's local-dev Docker targets, USER_UID and USER_GID build args are intentionally left without default values to force explicit UID/GID mapping during build time, preventing file permission issues in local development environments where container users need to match host user permissions for mounted volumes.

Applied to files:

• container/Dockerfile
• container/Dockerfile.vllm
• container/Dockerfile.sglang
• container/Dockerfile.trtllm

📚 Learning: 2025-11-14T01:09:35.244Z

Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 4323
File: components/src/dynamo/vllm/main.py:197-205
Timestamp: 2025-11-14T01:09:35.244Z
Learning: In components/src/dynamo/vllm/main.py, keivenchang considers temporary directory cleanup from tempfile.mkdtemp() to be LOW PRIORITY for production because containerized deployment patterns automatically clean up temp directories when containers are destroyed, mitigating resource leak concerns.

Applied to files:

• container/Dockerfile.vllm

📚 Learning: 2025-05-28T22:54:46.875Z

Learnt from: grahamking
Repo: ai-dynamo/dynamo PR: 1177
File: container/Dockerfile.vllm:102-105
Timestamp: 2025-05-28T22:54:46.875Z
Learning: In Dockerfiles, when appending to environment variables that may not exist in the base image, Docker validation will fail if you reference undefined variables with ${VARIABLE} syntax. In such cases, setting the environment variable directly (e.g., ENV CPATH=/usr/include) rather than appending is the appropriate approach.

Applied to files:

• container/Dockerfile.vllm

📚 Learning: 2025-06-11T21:18:00.425Z

Learnt from: julienmancuso
Repo: ai-dynamo/dynamo PR: 1474
File: deploy/cloud/operator/internal/controller/dynamocomponent_controller.go:1302-1306
Timestamp: 2025-06-11T21:18:00.425Z
Learning: In the Dynamo operator, the project’s preferred security posture is to set a Pod-level `PodSecurityContext` with `runAsUser`, `runAsGroup`, and `fsGroup` all set to `1000`, and then selectively override the user at the individual container level (e.g., `RunAsUser: 0` for Kaniko) when root is required.

Applied to files:

• container/Dockerfile.vllm

🔇 Additional comments (4)

container/Dockerfile.trtllm (1)

381-386: Late-binding DYNAMO_COMMIT_SHA in runtime stage matches the caching goal.

Placing ARG DYNAMO_COMMIT_SHA and the corresponding ENV right before ENTRYPOINT ensures the commit SHA is still exposed in the final image while limiting cache invalidation to only the last layer(s) when the SHA changes. This aligns well with the PR’s objective without impacting earlier expensive steps.

container/Dockerfile.vllm (1)

306-309: Consistent late-binding of DYNAMO_COMMIT_SHA for both runtime and dev targets looks solid.

Deferring ARG DYNAMO_COMMIT_SHA and ENV DYNAMO_COMMIT_SHA to the tail of the runtime and dev stages achieves the desired effect: commit changes only invalidate the final layers while still making the SHA available in the running containers. Mirroring this pattern across stages and with the other framework Dockerfiles keeps the build configuration predictable.

Also applies to: 378-381

container/Dockerfile (1)

427-430: Late DYNAMO_COMMIT_SHA binding in the dev image aligns with the multi-Dockerfile pattern.

Adding ARG DYNAMO_COMMIT_SHA and exporting it via ENV at the end of the dev stage keeps the SHA available in the dev container while avoiding unnecessary cache busting of earlier layers when the commit changes. This also keeps the base Dockerfile consistent with the framework-specific ones.

container/Dockerfile.sglang (1)

386-389: SGLang runtime and dev stages now correctly late-bind DYNAMO_COMMIT_SHA.

The added ARG/ENV DYNAMO_COMMIT_SHA blocks at the end of both runtime and dev stages give you the commit ID in the running containers while limiting cache invalidation to the final layers. The pattern matches the other Dockerfiles, which helps keep the container stack consistent and easier to reason about.

Also applies to: 498-501