Skip to content

ci(checkpoint): cherry-pick mount shared model-cache PVC in DynamoCheckpoint tests (#12400) - #12593

Merged
pvijayakrish merged 1 commit into
release/1.4.0from
schwinns/cherrypick-12400-release-1.4.0
Aug 4, 2026
Merged

ci(checkpoint): cherry-pick mount shared model-cache PVC in DynamoCheckpoint tests (#12400)#12593
pvijayakrish merged 1 commit into
release/1.4.0from
schwinns/cherrypick-12400-release-1.4.0

Conversation

@galletas1712

@galletas1712 galletas1712 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Cherry-picks merged #12400 (4bd68c399708004acd8d43a281deabb560178c4a) onto release/1.4.0.

  • Mounts the shared model-cache PVC in DynamoCheckpoint CI deployments
  • Avoids cold model downloads during vLLM, SGLang, and TensorRT-LLM checkpoint tests
  • Signed-off cherry-pick for DCO compliance

Main PR

#12400

Test plan

  • Release-branch CI passes
  • Confirm DynamoCheckpoint jobs use the shared model-cache PVC
  • Confirm no merge dependencies with other cherry-picks

Open in Devin Review

#12400)

Signed-off-by: Schwinn Saereesitthipitak <schwinns@nvidia.com>
@galletas1712
galletas1712 requested review from a team as code owners August 3, 2026 22:10
@github-actions github-actions Bot added ci Issues/PRs that reference CI build/test actions labels Aug 3, 2026

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

Open in Devin Review

Comment on lines +293 to +295
if model_cache_pvc:
mount = model_cache_mount or "/models"
deployment_spec.mount_model_cache_pvc(model_cache_pvc, mount)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Shared model cache is never actually attached in checkpoint tests, and TensorRT-LLM loses its model-storage setting

The shared model cache is attached using an older manifest format (mount_model_cache_pvc at tests/deploy/test_dynamocheckpoint.py:295) that the newer deployment format used by these tests does not accept, so the cache is silently dropped while the TensorRT-LLM model-storage setting is also removed.
Impact: Checkpoint CI jobs still download models from HuggingFace and the TensorRT-LLM restore test can lose its model files between checkpoint and restore, causing slow or failing runs.

Schema mismatch: mount_model_cache_pvc emits v1alpha1-only fields into a v1beta1 DGD

_new_checkpoint_spec loads examples/backends/<fw>/deploy/v1beta1/agg.yaml (see tests/deploy/test_dynamocheckpoint.py:237 and backend.manifest), so DeploymentSpec._schema is v1beta1.

DeploymentSpec.mount_model_cache_pvc (tests/utils/managed_deployment.py:494-512) writes:

  • spec.pvcs — exists only in the v1alpha1 CRD (deploy/operator/config/crd/bases/nvidia.com_dynamographdeployments.yaml:309, inside the v1alpha1 section); the v1beta1 section has no pvcs.
  • spec.envs — v1beta1 uses spec.env.
  • per-service volumeMounts: [{name, mountPoint}] via ServiceSpec.add_pvc_mount (tests/utils/managed_deployment.py:184-188); v1beta1 explicitly replaced per-component envs/volumeMounts with podTemplate (see the v1beta1 component description in the CRD).

Unknown fields are pruned by the CRD structural schema, so the PVC is never referenced or mounted. Meanwhile the new skip at tests/deploy/test_dynamocheckpoint.py:278-279 removes the TRTLLM HF_HOME=<snapshot PVC> container env whenever --model-cache-pvc is passed, relying on the deployment-level HF_HOME that is also pruned — leaving the pod with no HF_HOME at all, which the previous comment says is required to preserve model files across restore.

The pre-existing users of this helper (tests/deploy/conftest.py:363-367) operate on examples/backends/*/deploy/*.yaml, which are v1alpha1, which is why the helper works there.

Prompt for agents
DeploymentSpec.mount_model_cache_pvc in tests/utils/managed_deployment.py only emits v1alpha1 fields (spec.pvcs, spec.envs, per-service volumeMounts with mountPoint). The DynamoCheckpoint tests build their spec from examples/backends/<framework>/deploy/v1beta1/agg.yaml, so the schema is v1beta1, where those fields do not exist (spec.pvcs is absent, envs/volumeMounts were replaced by podTemplate). As a result the shared model-cache PVC is pruned by the CRD and never mounted, and because _new_checkpoint_spec now skips the TRTLLM container-level HF_HOME whenever --model-cache-pvc is set, the TRTLLM pods end up with no HF_HOME at all, losing the snapshot-PVC model preservation the previous code relied on. Make mount_model_cache_pvc schema-aware: for v1beta1, add a persistentVolumeClaim volume to each component's podTemplate.spec.volumes, a corresponding volumeMounts entry (mountPath) on the main container, and set HF_HOME either via spec.env or per-container env. Alternatively, gate the checkpoint test's cache mounting on the schema and keep the TRTLLM HF_HOME when the mount cannot be applied.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment thread .github/workflows/pr.yaml
Comment on lines +1535 to +1537
# Mount the shared model cache only when both endpoint vars are set
# (matches the PV/PVC creation gate); otherwise workers download from HF.
model_cache_pvc: ${{ vars.AZURE_MODEL_CACHE_SERVER != '' && vars.AZURE_MODEL_CACHE_PATH != '' && 'model-cache' || '' }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Shared cache mount is read-only NFS while HF_HOME points at it

mount_model_cache_pvc (tests/utils/managed_deployment.py:494-512) sets HF_HOME to the mount path, and the PV created by .github/actions/setup-dynamo-operator/action.yml:337-377 is an NFS export shared across CI runs. If the HuggingFace client needs to write locks/metadata under HF_HOME (typical when a model is not fully present in the cache), checkpoint pods will attempt writes into the shared export. Worth confirming the export is writable or that the cache always contains Qwen3-0.6B, otherwise the checkpoint jobs may fail differently than the regular deploy tests, which have longer soak history with this mount.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@datadog-official

datadog-official Bot commented Aug 3, 2026

Copy link
Copy Markdown

🎯 Code Coverage (details)
Patch Coverage: 0.00%
Overall Coverage: 44.86% (-5.63%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 2c0a752 | Docs | Datadog PR Page | Give us feedback!

@pvijayakrish
pvijayakrish merged commit 11ba7dd into release/1.4.0 Aug 4, 2026
183 of 186 checks passed
@pvijayakrish
pvijayakrish deleted the schwinns/cherrypick-12400-release-1.4.0 branch August 4, 2026 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

actions ci Issues/PRs that reference CI build/test size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants