[TRTLLMINF-213][infra] Artifactory container image migration - #16970
[TRTLLMINF-213][infra] Artifactory container image migration#16970tburt-nv wants to merge 10 commits into
Conversation
|
/bot run --extra-stage "Build-Docker-Images" |
|
PR_Github #62283 [ run ] triggered by Bot. Commit: |
|
PR_Github #62283 [ run ] completed with state
|
|
/bot run |
|
PR_Github #62349 [ run ] triggered by Bot. Commit: |
|
PR_Github #62349 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #62603 [ run ] triggered by Bot. Commit: |
|
PR_Github #62603 [ run ] completed with state
|
62dfabd to
3292911
Compare
|
/bot run --disable-fail-fast |
3292911 to
6f0f180
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #62823 [ run ] triggered by Bot. Commit: |
|
PR_Github #62823 [ run ] completed with state |
6f0f180 to
c450800
Compare
|
/bot run --extra-stage "Build-Docker-Images" |
|
/bot run --extra-stage "Build-Docker-Images" --disable-fail-fast |
|
PR_Github #62833 [ run ] triggered by Bot. Commit: |
|
PR_Github #62833 [ run ] completed with state
|
|
/bot run --extra-stage "Build-Docker-Images" --disable-fail-fast |
|
PR_Github #62864 [ run ] triggered by Bot. Commit: |
|
PR_Github #62864 [ run ] completed with state
|
9053ff3 to
112382a
Compare
|
/bot run |
|
PR_Github #63076 [ run ] triggered by Bot. Commit: |
|
PR_Github #63076 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/bot run --disable-fail-fast |
|
PR_Github #64100 [ run ] triggered by Bot. Commit: |
|
PR_Github #64100 [ run ] completed with state
|
Signed-off-by: Tyler Burt <195370667+tburt-nv@users.noreply.github.com>
Signed-off-by: Tyler Burt <195370667+tburt-nv@users.noreply.github.com>
Signed-off-by: Tyler Burt <195370667+tburt-nv@users.noreply.github.com>
Signed-off-by: Tyler Burt <195370667+tburt-nv@users.noreply.github.com>
Signed-off-by: Tyler Burt <195370667+tburt-nv@users.noreply.github.com>
Signed-off-by: Tyler Burt <195370667+tburt-nv@users.noreply.github.com>
Signed-off-by: Tyler Burt <195370667+tburt-nv@users.noreply.github.com>
Signed-off-by: Tyler Burt <195370667+tburt-nv@users.noreply.github.com>
Signed-off-by: Tyler Burt <195370667+tburt-nv@users.noreply.github.com>
Signed-off-by: Tyler Burt <195370667+tburt-nv@users.noreply.github.com>
8a89113 to
9180ca4
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/bot run --disable-fail-fast |
|
PR_Github #64137 [ run ] triggered by Bot. Commit: |
|
PR_Github #64137 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64359 [ run ] triggered by Bot. Commit: |
|
PR_Github #64359 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
brnguyen2
left a comment
There was a problem hiding this comment.
Migration itself looks mechanically complete for the Jenkins/K8s side (image refs, imagePullSecrets, credential IDs). Two things worth settling before merge, plus a couple of leftovers.
Enroot URI conversion is now host-specific — is that deliberate? [L0_Test.groovy:1806](https://github.com/NVIDIA/TensorRT-LLM/pull/16970/files#diff-3efa8a6083a90a6f716e4d4bc81710c8104f8bfc5ae642e9fef44df8061e6b7dR1806) and :1011 replaced the unconditional urm.nvidia.com/ → urm.nvidia.com# rewrite with an artifactory-only one. env.dockerImage is a job parameter, so a run pointed at a urm.nvidia.com/... image now gets docker://urm.nvidia.com/sw-tensorrt-docker/..., which enroot parses as a Docker Hub path (no # → default registry).
If the intent is that urm.nvidia.com is being retired under TRTLLMINF-213 and any urm-targeted run is dead regardless, that's a reasonable call — just say so in a comment and I'll drop this. Two things make me unsure that's the case: the DLFW_IMAGE constant near the top of this file still points at urm.nvidia.com/docker/nvidia/pytorch, and run_disagg.sh deliberately kept both rewrites in this same PR. If urm stays pullable through a transition period, the narrowed rewrite turns a working override into a confusing enroot parse failure (or a Docker Hub lookup) instead of a clean auth error. A generic first-segment host/ → host# costs nothing and is retirement-agnostic either way.
Undisclosed leftovers. The PR description doesn't mention the PIP_INDEX_URL behavior change in Dockerfile.multi beyond "PyPI mirror support" — see inline. Also still on urm.nvidia.com/sw-tensorrt-docker: examples/layer_wise_benchmarks/slurm_init_containers.sh:29 and jenkins/scripts/perf/local/configs/example.conf:38, both user-facing examples that break once images stop being published to the old repo. If the old repo stays readable for a while, fine to defer — but say so in the description. (Same question underneath: what's the actual timeline for urm going away?)
Docs. docker/README.md is updated; nothing else in docs/ seems to name the registry, so I think that's covered.
Ticket tag (TRTLLMINF-213) is correct for this. The slurm_job_result.txt delimiter change (' ' → '|') is a good fix but is unrelated to the registry migration and isn't mentioned in the description — worth one line there so a future bisect knows why it moved.
Non-blocking overall; the enroot URI question is the one I'd want answered before merge.
| def container = LLM_DOCKER_IMAGE | ||
| if (cluster.containerRuntime.toString() == "ENROOT") { | ||
| container = LLM_DOCKER_IMAGE | ||
| .replace("${ARTIFACTORY_DOCKER_HOST}/", "${ARTIFACTORY_DOCKER_HOST}#") |
There was a problem hiding this comment.
This narrows the previous unconditional urm.nvidia.com/ → urm.nvidia.com# rewrite to the artifactory host only. LLM_DOCKER_IMAGE comes from env.dockerImage, which is an overridable job parameter — a run pointed at a urm.nvidia.com/... image now yields docker://urm.nvidia.com/sw-tensorrt-docker/tensorrt-llm:tag. Enroot's URI grammar is docker://[USER@][REGISTRY#]IMAGE[:TAG], so without the # the whole thing is treated as an image path on the default registry.
Is that intentional — i.e. urm is being retired as part of this migration, so a urm-targeted run would fail on auth/availability anyway and there's no point preserving the rewrite? If so, a one-line comment here saying that would save the next reader the same question. What makes me unsure: DLFW_IMAGE near the top of this file still resolves against urm.nvidia.com/docker/..., and jenkins/scripts/perf/local/run_disagg.sh kept both rewrites in this PR — so at least some urm paths are expected to keep working.
If urm stays pullable during a transition, a generic first-segment host/ → host# rewrite handles both without a host list and survives the next registry move too. Same at [L0_Test.groovy:1014](https://github.com/NVIDIA/TensorRT-LLM/pull/16970/files#diff-3efa8a6083a90a6f716e4d4bc81710c8104f8bfc5ae642e9fef44df8061e6b7dR1014).
There was a problem hiding this comment.
urm.nvidia.com will not be used for pulling images after this PR is merged.
| ARG BUILD_WHEEL_ARGS="--clean" | ||
| ARG BUILD_WHEEL_SCRIPT="scripts/build_wheel.py" | ||
| RUN --mount=type=cache,target=/root/.cache/pip --mount=type=cache,target=${CCACHE_DIR} \ | ||
| if [ -n "$GITHUB_MIRROR" ]; then \ |
There was a problem hiding this comment.
Using GITHUB_MIRROR as the toggle for PIP_INDEX_URL overloads a flag whose documented meaning is "proxy for GitHub downloads". An external user who sets GITHUB_MIRROR to their own GitHub proxy silently gets their pip index redirected to an NVIDIA-internal host they can't reach, and the build fails at wheel install with a confusing error. It also hardcodes urm.nvidia.com in the PR that migrates off urm.nvidia.com — so this will need touching again (unless the pypi-remote side of urm is staying put; if so, worth a comment saying the docker repo and the pypi mirror have different fates).
Suggest a dedicated ARG PIP_INDEX_URL="" plumbed from docker/Makefile (mirroring the existing $(if $(GITHUB_MIRROR), --build-arg ...) pattern at docker/Makefile:104), with [jenkins/BuildDockerImage.groovy:302](https://github.com/NVIDIA/TensorRT-LLM/pull/16970/files#diff-3b1526ac2c26deffd5ac687c4acdc418ce272d866ad0867755e39072ebc8fef9R302) passing both. Same comment for the duplicate block at line 131.
There was a problem hiding this comment.
It's already overloaded, and I don't intend to refactor this now.
| trtllm_utils.llmExecStepWithRetry(this, script: "docker login urm.nvidia.com -u ${USERNAME} -p ${PASSWORD}") | ||
| // Read-write artifactory credentials (image push) | ||
| withCredentials([usernamePassword(credentialsId: "aws-artifactory-credentials", usernameVariable: 'USERNAME', passwordVariable: 'PASSWORD')]) { | ||
| trtllm_utils.llmExecStepWithRetry(this, script: "docker login artifactory.nvidia.com -u ${USERNAME} -p ${PASSWORD}") |
There was a problem hiding this comment.
The pull-side login in L0_Test.groovy moved to --password-stdin, but this push-side one still passes -p ${PASSWORD} on the command line — docker itself warns about this on every invocation, and it's a line this PR is already touching. Same one-liner applies:
trtllm_utils.llmExecStepWithRetry(this, script: "set +x; echo \"\$PASSWORD\" | docker login artifactory.nvidia.com -u \"\$USERNAME\" --password-stdin")There was a problem hiding this comment.
This is pre-existing behavior I'm not interested in refactoring as part of this PR. We can handle this enhancement separately.
|
/bot run --disable-fail-fast |
|
PR_Github #64636 [ run ] triggered by Bot. Commit: |
|
PR_Github #64636 [ run ] completed with state
|
Dev Engineer Review
urm.nvidia.comtoartifactory.nvidia.com.QA Engineer Review
No test changes.
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.