aws / aws/deep-learning-containers
autocurrency: is_newer_version() aborts on non-numeric version segments (e.g. 0.5.13+dlc1)
- Dominant language
- Python
- Stars
- 1.2k
- Forks
- 559
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 102
Description
## Summary
`is_newer_version()` in `scripts/ci/autocurrency/utils.sh` forces every version segment through bash base-10 arithmetic. Any segment that is not purely numeric either **aborts the script** under `set -u`, or is **silently evaluated as 0** without it.
This function is the gate that decides whether the nightly upstream-release tracker opens an auto-update PR, so a failure here silently stops currency updates for a framework.
## Environment
- Repo version: `main` @ [`a09e9fd`](https://github.com/aws/deep-learning-containers/commit/a09e9fdc74956fdf0ae87f01c788fc8936fa6ea0)
- Runs on the CI host, not inside a container — `_scheduled.check-upstream-releases.yml`
- Reproduced on bash 3.2.57 (macOS) and bash 5.x; not version-specific
## Steps to reproduce
```bash
git clone https://github.com/aws/deep-learning-containers.git
cd deep-learning-containers && git checkout a09e9fd
bash -c 'source scripts/ci/autocurrency/utils.sh; is_newer_version "0.5.14" "0.5.13+dlc1"'
```
Actual:
```
scripts/ci/autocurrency/utils.sh: line 82: dlc1: unbound variable
```
Expected: return 0 (upstream `0.5.14` is newer than `0.5.13+dlc1`).
## Cause
[`utils.sh#L80-L82`](https://github.com/aws/deep-learning-containers/blob/a09e9fdc74956fdf0ae87f01c788fc8936fa6ea0/scripts/ci/autocurrency/utils.sh#L80-L82):
```bash
for i in 0 1 2; do
local u_seg=$((10#${u_parts[$i]}))
local c_seg=$((10#${c_parts[$i]}))
```
Splitting `0.5.13+dlc1` on `.` yields the third segment `13+dlc1`. Inside `$(( ))`, bash parses that as `10#13 + dlc1` and tries to resolve `dlc1` as a variable. `utils.sh` sets `set -euo pipefail` at the top, so under `set -u` this aborts.
## Impact
`check-upstream-releases.sh` runs each framework inside a `set -euo pipefail` subshell ([L102-L104](https://github.com/aws/deep-learning-containers/blob/a09e9fdc74956fdf0ae87f01c788fc8936fa6ea0/scripts/ci/autocurrency/check-upstream-releases.sh#L102-L104)), so the abort kills that framework's entire update block. The outer loop reports only:
```
::warning::sglang: Processing failed with exit code 1
```
and the job exits 1 — with nothing pointing at the version string as the cause.
The `+dlc` suffix is a live convention in this repo. It is documented in `check_framework_version_currency.py` as the contract `framework_version == "[+dlc]"`, and is present in shipped configs today:
- `.github/config/image/sglang/ec2-amzn2023.yml` → `0.5.14+dlc1`
- `.github/config/image/sglang/sagemaker-amzn2023.yml` → `0.5.14+dlc1`
- `.github/config/image/vllm/hyperpod-amzn2023.yml` → `0.20.0.dev361`
`is_newer_version` reads `metadata.framework_version` from `config_files[0]` of the tracked framework. Those entries happen to be plain numeric right now, so the nightly job survives by luck — reordering `config_files` in `autocurrency-tracker.yml`, or landing a `+dlc` rebuild on a tracked config, breaks it immediately.
## Second defect: segments past the third are dropped
The loop is fixed at `for i in 0 1 2`, so a four-segment release compares equal to its three-segment predecessor:
```bash
$ bash -c 'source scripts/ci/autocurrency/utils.sh; set +u
is_newer_version "1.0.0.1" "1.0.0"; echo "rc=$?"'
rc=1 # "not newer" — no update PR would ever be raised
```
Also note that without `set -u` the first defect degrades quietly rather than loudly: bash resolves the unknown identifier to `0`, so the comparison silently uses the wrong number.
## Suggested fix
Compare on the numeric release portion only — keep the leading digits of each segment and stop at the first suffix, so `+dlc1`, `.post1` and `.dev361` never reach the arithmetic — and compare every segment present rather than the first three. A malformed version should produce a clear diagnostic instead of `unbound variable`.
**Correction (edited):** this issue originally said no PR had been opened. I have since opened #6519 with the patch, so that line was no longer true and has been replaced. I am aware CONTRIBUTING.md asks external contributors not to open PRs — please close #6519 without review if that is the standing policy and treat this issue as the report. Patch also on my fork: https://github.com/Adityaj0/deep-learning-containers/pull/4
Contributor guide
Research direction
Start with scripts/ci/autocurrency/utils.sh and reproduce the reported calls to is_newer_version; then read its use from scripts/ci/autocurrency/check-upstream-releases.sh and the version contract in check_framework_version_currency.py. Done means suffixes such as +dlc1 and .dev361 no longer abort or compare incorrectly, versions with more than three segments are handled, and malformed versions produce a clear diagnostic.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- bash
- Domain
- ci-cd, tooling
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 30/100