chore: CVE upgrades for wandb, gitpython and cleanups - #1251
Conversation
|
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:
📝 WalkthroughWalkthroughThe pull request updates GitPython and W&B versions across project requirements and Docker images. It changes CVE cleanup to POSIX ChangesDependency and runtime image alignment
Policy WASM build
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 `@docker/scripts/cve-cleanup.sh`:
- Around line 90-93: Update the cleanup entries in the script around the
GitPython and JupyterLab removals to target the runtime Python site-packages
directory rather than hardcoded Python 3.12 paths. Ensure Dockerfile.nmp-api and
Dockerfile.safe-synthesizer-tasks provide or resolve the correct Python 3.13
path, and add build assertions verifying the targeted packages and dist-info
files are absent afterward.
🪄 Autofix
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: ee0a055f-8b30-4b4e-9beb-d7a9de09ee6f
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
docker/Dockerfile.nmp-apidocker/Dockerfile.nmp-customizer-tasksdocker/Dockerfile.nmp-unsloth-trainingdocker/Dockerfile.safe-synthesizer-tasksdocker/automodel/Dockerfile.nmp-automodel-basedocker/automodel/no_override_requirements.txtdocker/rl/Dockerfile.nmp-rl-basedocker/scripts/cve-cleanup.shdocker/unsloth/no_override_requirements.txtpyproject.tomlthird_party/requirements-main.txt
| rm -rf /usr/local/lib/python3.12/dist-packages/git \ | ||
| /usr/local/lib/python3.12/dist-packages/gitpython-*.dist-info | ||
| rm -rf /usr/local/lib/python3.12/dist-packages/jupyterlab \ | ||
| /usr/local/lib/python3.12/dist-packages/jupyterlab-*.dist-info |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Use the runtime Python path for the cleanup entries.
docker/Dockerfile.nmp-api and docker/Dockerfile.safe-synthesizer-tasks use Python 3.13, but Lines 90-93 only target /usr/local/lib/python3.12/dist-packages. These image builds can therefore skip the new GitPython and JupyterLab cleanup.
Discover the runtime site-packages directory or pass it from each image. Add a build assertion that the targeted files are absent.
Proposed fix
-rm -rf /usr/local/lib/python3.12/dist-packages/git \
- /usr/local/lib/python3.12/dist-packages/gitpython-*.dist-info
-rm -rf /usr/local/lib/python3.12/dist-packages/jupyterlab \
- /usr/local/lib/python3.12/dist-packages/jupyterlab-*.dist-info
+for site_packages in \
+ /usr/local/lib/python*/dist-packages \
+ /usr/local/lib/python*/site-packages; do
+ [ -d "$site_packages" ] || continue
+ rm -rf \
+ "$site_packages/git" \
+ "$site_packages"/gitpython-*.dist-info \
+ "$site_packages/jupyterlab" \
+ "$site_packages"/jupyterlab-*.dist-info
+done🤖 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 `@docker/scripts/cve-cleanup.sh` around lines 90 - 93, Update the cleanup
entries in the script around the GitPython and JupyterLab removals to target the
runtime Python site-packages directory rather than hardcoded Python 3.12 paths.
Ensure Dockerfile.nmp-api and Dockerfile.safe-synthesizer-tasks provide or
resolve the correct Python 3.13 path, and add build assertions verifying the
targeted packages and dist-info files are absent afterward.
626c525 to
7ee4f39
Compare
|
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
7ee4f39 to
38c16ba
Compare
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
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)
docker/rl/Dockerfile.nmp-rl-base (1)
202-205: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep W&B 0.28.2 in the RL lock
The RL
uv.lockcopied by this Dockerfile pinswandbto0.28.1. This command installs0.28.2outside the frozen dependency graph. Apply the same fix at line 344. Update the RL lock to0.28.2, or constrain and validate the direct install's complete dependency graph.🤖 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 `@docker/rl/Dockerfile.nmp-rl-base` around lines 202 - 205, Update the RL dependency setup around the direct wandb installation and the corresponding lock entry so wandb is consistently pinned to 0.28.2. Apply the same correction at the installation site around line 344, ensuring the copied uv.lock and both direct installs use the same resolved dependency graph rather than installing 0.28.2 outside the frozen lock.
🤖 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 `@docker/rl/Dockerfile.nmp-rl-base`:
- Around line 202-205: Update the RL dependency setup around the direct wandb
installation and the corresponding lock entry so wandb is consistently pinned to
0.28.2. Apply the same correction at the installation site around line 344,
ensuring the copied uv.lock and both direct installs use the same resolved
dependency graph rather than installing 0.28.2 outside the frozen lock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 77bfdfd4-72b5-44e5-aa9a-a4a4d1c2eb04
📒 Files selected for processing (2)
docker/base/Dockerfile.policy-wasmdocker/rl/Dockerfile.nmp-rl-base
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
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 `@script/build_policy_wasm.sh`:
- Around line 134-137: Update the download flow in the build policy script to
track a single absolute deadline across binary and checksum requests, rather
than relying on per-request --max-time and --retry-max-time values. Apply the
remaining time to each curl attempt and stop trying fallback sources when the
shared deadline is reached, reserving sufficient time for opa build within the
caller’s timeout.
🪄 Autofix
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: d1c7ed92-79f5-40a0-9627-65440a4c3899
📒 Files selected for processing (2)
docker/base/Dockerfile.policy-wasmscript/build_policy_wasm.sh
| if curl -fsSL --retry 3 --retry-delay 2 --retry-all-errors \ | ||
| --connect-timeout 10 --max-time 30 --retry-max-time 45 "${url}" -o "${tmp_bin}" && \ | ||
| curl -fsSL --retry 3 --retry-delay 2 --retry-all-errors \ | ||
| --connect-timeout 10 --max-time 10 --retry-max-time 15 "${sha_url}" -o "${tmp_sha}"; then |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
# Verify curl retry-window behavior.
curl -fsSL https://curl.se/docs/manpage.html |
grep -A12 -F -- '--retry-max-time' | sed -n '1,20p'
# Inspect the configured per-source budget.
sed -n '123,146p' script/build_policy_wasm.shRepository: NVIDIA-NeMo/nemo-platform
Length of output: 2696
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- script structure and timeout-related lines ---'
sed -n '1,180p' script/build_policy_wasm.sh
printf '%s\n' '--- caller timeout references ---'
rg -n -C 3 'DEFAULT_BUILD_TIMEOUT_SECONDS|build_policy_wasm|timeout' . \
-g '*.py' -g '*.sh' -g '*.yaml' -g '*.yml' -g '*.toml' \
| head -240Repository: NVIDIA-NeMo/nemo-platform
Length of output: 22974
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- curl option documentation ---'
curl -fsSL https://curl.se/docs/manpage.html |
sed -n '/id="--retry-max-time"/,/id="--retry"/p' |
sed -n '1,100p'
printf '%s\n' '--- available local curl documentation ---'
curl --version
curl --help all | grep -E -A2 -B2 -- '--(max-time|retry-max-time|retry)' || trueRepository: NVIDIA-NeMo/nemo-platform
Length of output: 1561
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- policy_wasm.py locations and timeout definitions ---'
fd -i 'policy_wasm.py' .
rg -n -C 8 'DEFAULT_BUILD_TIMEOUT_SECONDS|build_policy_wasm|build_policy' \
$(fd -i 'policy_wasm.py' .) 2>/dev/null || true
printf '%s\n' '--- exact retry-max-time documentation text ---'
curl -fsSL https://curl.se/docs/manpage.html |
grep -n -i -A18 -B8 'retry-max-time' |
head -100Repository: NVIDIA-NeMo/nemo-platform
Length of output: 27101
🌐 Web query:
curl --retry-max-time current transfer allowed to finish --max-time resets on each retry official curl documentation
💡 Result:
In curl, the --max-time and --retry-max-time options function differently regarding how they handle time limits and retries [1][2][3]. --max-time: This option sets the maximum time allowed for a single, individual transfer attempt [1][4]. If you have enabled retries using the --retry option, the --max-time counter is reset for every new retry attempt [1][5][6]. Consequently, --max-time does not limit the total duration of the entire operation across all retries, only the duration of each specific attempt [1][2][3]. --retry-max-time: This option acts as a global timer for the entire retry process [2][3]. It limits the total time during which curl is allowed to perform retry attempts [2][3]. The timer starts before the first transfer attempt and continues to run through both the transfer attempts and the waiting periods (such as those caused by --retry-delay) between them [2][7][3]. If this timer reaches the specified limit, no further retries will be performed [2][3]. However, if a transfer is already in progress when the --retry-max-time limit is reached, that transfer is allowed to continue and complete, even if it exceeds the specified limit [2][7][3]. In summary, --max-time is reset on each retry attempt [1][6], while --retry-max-time sets a cumulative time limit for the entire retry lifecycle [2][3].
Citations:
- 1: https://github.com/curl/curl/blob/c37405cb/docs/cmdline-opts/max-time.md
- 2: https://linux.die.net/man/1/curl
- 3: Non-intuitive behavior of --retry and --retry-max-time used together curl/curl#17444
- 4: https://github.com/curl/everything-curl/blob/master/usingcurl/timeouts.md
- 5: https://github.com/curl/curl/blob/master/docs/cmdline-opts/max-time.md
- 6: max-time.d: clarify max-time sets max transfer time curl/curl#8879
- 7: docs: clarify retry-max-time timing check and sleep behavior curl/curl#21411
🏁 Script executed:
#!/bin/bash
set -eu
curl -fsSL https://curl.se/docs/manpage.html |
python3 - <<'PY'
import sys
from html.parser import HTMLParser
class Text(HTMLParser):
def __init__(self):
super().__init__()
self.capture = False
self.depth = 0
self.parts = []
def handle_starttag(self, tag, attrs):
attrs = dict(attrs)
if attrs.get("id") == "--retry-max-time":
self.capture = True
self.depth = 0
elif self.capture:
self.depth += 1
def handle_endtag(self, tag):
if self.capture:
if self.depth == 0:
self.capture = False
else:
self.depth -= 1
def handle_data(self, data):
if self.capture:
self.parts.append(" ".join(data.split()))
p = Text()
p.feed(sys.stdin.read())
print(" ".join(p.parts))
PYRepository: NVIDIA-NeMo/nemo-platform
Length of output: 238
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from html.parser import HTMLParser
from urllib.request import urlopen
class Text(HTMLParser):
def __init__(self):
super().__init__()
self.capture = False
self.depth = 0
self.parts = []
def handle_starttag(self, tag, attrs):
attrs = dict(attrs)
if attrs.get("id") == "--retry-max-time":
self.capture = True
self.depth = 0
elif self.capture:
self.depth += 1
def handle_endtag(self, tag):
if self.capture:
if self.depth == 0:
self.capture = False
else:
self.depth -= 1
def handle_data(self, data):
if self.capture:
text = " ".join(data.split())
if text:
self.parts.append(text)
with urlopen("https://curl.se/docs/manpage.html") as response:
parser = Text()
parser.feed(response.read().decode())
print(" ".join(parser.parts))
PYRepository: NVIDIA-NeMo/nemo-platform
Length of output: 2450
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '83,125p' services/core/auth/src/nmp/core/auth/app/embedded_pdp/policy_wasm.pyRepository: NVIDIA-NeMo/nemo-platform
Length of output: 1650
Use a shared download deadline.
--retry-max-time does not stop an active transfer, and --max-time resets on each retry. The binary and checksum requests can consume about 100 seconds per source. Two fallback sources can exceed the caller's 120-second timeout before opa build starts.
Track one absolute deadline across all sources and reserve time for opa build.
🤖 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 `@script/build_policy_wasm.sh` around lines 134 - 137, Update the download flow
in the build policy script to track a single absolute deadline across binary and
checksum requests, rather than relying on per-request --max-time and
--retry-max-time values. Apply the remaining time to each curl attempt and stop
trying fallback sources when the shared deadline is reached, reserving
sufficient time for opa build within the caller’s timeout.
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
Signed-off-by: Matt Kornfield <mkornfield@nvidia.com>
1 participant
Summary
Related Issue
Changes
Type of Change
Quality Gates
Verification
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted validation:
Summary by CodeRabbit
Bug Fixes
Maintenance