[CI] Retry transient Python package downloads - #7144
Conversation
Greptile SummaryThis PR adds a shared fixed-attempt command wrapper and applies it to Python package installation steps across workflows and composite actions to tolerate transient download failures.
Confidence Score: 5/5The PR appears safe to merge; no concrete changed-code failure was identified in the retry helper or its current CI integrations. The wrapper preserves command arguments and final exit codes, current workflows make the helper available before use, container paths resolve to explicit read-only mounts, and the retried installation paths are restart-safe or self-healing. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CI package installation step] --> B[retry-command.sh]
B --> C[Run pip, uv, or Isaac Lab installer]
C -->|Success| D[Continue CI job]
C -->|Failure and attempts remain| E[Wait three seconds]
E --> C
C -->|Third failure| F[Return final command status]
Reviews (1): Last reviewed commit: "[CI] Retry transient Python package down..." | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
The shared retry wrapper consistently retries CI package-install commands and preserves final exit status, but one composite action locates the helper through the caller’s workspace rather than its own action directory, reducing action portability.
- Design and architecture: Centralizing retry behavior in
.github/actions/_lib/retry-command.shavoids duplicated workflow logic. However,upload-omni-github-test-resultsshould resolve this repository-owned helper relative to$GITHUB_ACTION_PATH, as it already does for its converter and schema resources. - API: No Python or CLI API changes are introduced. The upload-results composite action’s consumption contract is affected: using
$GITHUB_WORKSPACEassumes Isaac Lab is checked out at the workspace root and can fail when the action is consumed externally or checkout uses a custom path. - Implementation: The wrapper correctly forwards arguments with
"$@", captures failures, retries up to three times, and returns the final status. Update the upload-results action to invoke$GITHUB_ACTION_PATH/../_lib/retry-command.shso its helper follows the action package rather than the caller’s checkout layout.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| schema_venv="${RUNNER_TEMP}/omni-github-jsonschema-venv" | ||
| python3 -m venv "$schema_venv" | ||
| "$schema_venv/bin/python" -m pip install -q jsonschema | ||
| bash "$GITHUB_WORKSPACE/.github/actions/_lib/retry-command.sh" \ |
There was a problem hiding this comment.
🔵 Suggestion · Design Architecture — Action resolves helper outside its action path
This action resolves every other resource through $GITHUB_ACTION_PATH (converter script and schema) and otherwise never reads the workspace. Sourcing the retry helper via $GITHUB_WORKSPACE/.github/actions/_lib/... introduces a new assumption that an IsaacLab checkout exists at the workspace root, which breaks if the action is consumed from another repo or with actions/checkout path:. Prefer $GITHUB_ACTION_PATH/../_lib/retry-command.sh.
|
overall the logic is clean and the separate file makes it easy to update if we needed to. |
| fi | ||
|
|
||
| readonly max_attempts=3 | ||
| readonly retry_delay_seconds=3 |
There was a problem hiding this comment.
@nv-apoddubny had mentioned adding a delay, if readonly retry_delay_seconds=3 is good with him I do not have any objections
|
should we add the same retry to build.sh @StafaH ? https://github.com/isaac-sim/IsaacLab/actions/runs/32083873111/job/95552207335?pr=7144#step:5:77 - I see that this guy failed with a similar problem |
…ci/retry-python-downloads
AntoineRichard
left a comment
There was a problem hiding this comment.
AI Review (Codex)
This review was generated by OpenAI Codex and verified against PR head 1f36f71.
Important findings
-
The CI helper does not retry failed install commands.
.github/actions/_lib/with-python-package-retries.sh:14-20 only sets PIP_RETRIES and UV_HTTP_RETRIES before executing the command once. This does not match the PR description of up to three command attempts. The current isaaclab_newton and rendering-correctness jobs demonstrate the gap: each pip process exhausted all 12 internal HTTP retries on repeated files.pythonhosted.org 502 responses, then the job failed without another command attempt.
Please define a bounded total retry budget. One option is a command-level loop with fewer internal HTTP retries; simply nesting three attempts around 12 HTTP retries could make one package failure take roughly 24 minutes. A CI wheel cache or wheelhouse would be more robust for these bootstrap dependencies.
-
The PR removes PhysX launch coverage for two Franka-soft tasks.
source/isaaclab_tasks/test/core/test_environments_isaacsim_physx.py:28-29 adds Isaac-Lift-Soft-Franka and Isaac-Lift-Soft-Franka-Camera to _COVERED_TASKS. However, the referenced rendering path constructs only FrankaSoftCameraEnvCfg and explicitly skips physics_backend == physx. The non-camera task has no replacement test, so neither exclusion is actually covered for this PhysX path.
Please remove these unrelated exclusions, or add targeted replacement coverage. If a known teardown issue requires suppression, use an explicit skip with the bug reference rather than marking the tasks as covered.
CI diagnosis
- isaaclab_newton: pytest-mock metadata download failed after persistent HTTP 502 responses; tests never started.
- rendering-correctness: pytetwild metadata download failed after persistent HTTP 502 responses; tests never started.
- record-video: four tests passed; test_multiple_recorders_simultaneous narrowly missed the motion threshold (0.096 < 0.1). This test is unchanged by the PR and appears flaky/unrelated.
Verification
- Focused CLI tests: 78 passed, 1 skipped.
- ./isaaclab.sh -f: all hooks passed.
- Changed shell scripts passed bash syntax validation.
- git diff --check passed.
GPU and rendering tests were not reproduced locally.
# Description Every CI test run installed the pytest harness (`pytest`, `pytest-mock`, `junitparser`, `flaky`, `coverage>=7.6.1`) from PyPI before it could start — once per test job in `run_tests.sh`, and again on each multi-GPU shard host in `multi_gpu_shard_runner.sh`. That puts a network round-trip on the critical path of every lane and makes runs fail on transient PyPI errors. This bakes those packages into the image instead: - **`docker-build/action.yml`** gains a step that layers the pytest deps onto the freshly built image (`FROM <image-tag>` on stdin, no build context). It only runs when a real build happened — cache hits already carry the layer. The layer installs as `root` and restores the image's own default `USER` afterwards, read back from `docker image inspect`. - **`_lib/compute-deps-hash/action.yml`** now hashes `.github/actions/docker-build/action.yml`, so editing the package list invalidates the local deps-cache tag and forces a rebuild instead of silently reusing an image without the new packages. - **`run_tests.sh`** and **`multi_gpu_shard_runner.sh`** drop their now-redundant installs. The `with-python-package-retries.sh` wrapper added in #7144 stays in place for the remaining `TEST_EXTRA_PIP_PACKAGES` / `TEST_EXTRA_UV_PACKAGES` installs, which are still per-run by design. The deps stay out of the tracked `Dockerfile.*` files on purpose: those build the local dev containers that ship via `publish-images.yaml`, and the pytest harness is a CI-only concern. Fixes # (issue) ## Type of change - Bug fix (non-breaking change which fixes an issue) ## Validation - `uv run isaaclab -f` - `bash -n` on both modified shell scripts - YAML parse of both modified composite actions - `uv run --no-project python tools/changelog/cli.py check develop` ## Screenshots N/A — CI-only change. ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [ ] I have made corresponding changes to the documentation — N/A, no user-facing docs cover the CI image build - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works — N/A, CI itself is the test: every lane that pulls the image must still collect and run pytest - [ ] I have added a changelog fragment under `source/<pkg>/changelog.d/` for every touched package — N/A, no `source/<pkg>/` files are touched; `tools/changelog/cli.py check develop` passes - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there --------- Co-authored-by: Antoine RICHARD <antoiner@nvidia.com>
Description
Retries CI Python package installs up to three times with a three-second delay to tolerate transient PyPI connectivity failures.
Validation
uv run isaaclab -f