Skip to content

ci: share the set_env_* helper functions in one library - #417

Closed
lvyufeng wants to merge 1 commit into
flagos-ai:mainfrom
lvyufeng:refactor/set-env-common
Closed

lvyufeng wants to merge 1 commit into
flagos-ai:mainfrom
lvyufeng:refactor/set-env-common

Conversation

@lvyufeng

Copy link
Copy Markdown
Collaborator

AI Agent Information

Summary

Adds .github/scripts/lib/set_env_common.sh with the five helpers the seven platform provisioning scripts each carried their own copy of (pip_retry, flag_gems_installed, install_flag_gems, strip_vendor_paths, venv_is_usable), plus the 21-line FlagGems VCS-install rationale that was duplicated verbatim in six of them. Every script sources the library and drops its local copies; the per-platform main flows are unchanged. Net -676/+285 across the seven scripts.

Change Type

  • Bug Fix
  • New Feature
  • Performance Optimization
  • Refactoring
  • Documentation
  • Testing
  • CI/Infrastructure
  • Breaking Change

Platforms Affected

  • CUDA
  • MetaX
  • Ascend
  • PPU
  • Platform-agnostic (all platforms)

Problem Analysis

What was broken/missing?

Each of .github/scripts/set_env_{cuda,ascend,dcu,gcu,metax,musa,ppu}.sh carried its own copy of:

  • pip_retry -- five slightly different spellings (interpreter, timeout 300 vs 600, --no-cache-dir on/off);
  • flag_gems_installed and install_flag_gems -- six copies each, two variants;
  • strip_vendor_paths -- three identical copies;
  • venv_is_usable -- two copies;
  • the 21-line "FlagGems install is a VCS install…" rationale -- six copies verbatim.

A fix had to be applied seven times and was easy to miss.

Why did it happen?

Each new platform job copied the previous script and patched the vendor-specific lines; the shared skeleton was never factored out.

Investigation process:

  1. Extracted every function body across the seven scripts and counted distinct variants (pip_retry 5, flag_gems_installed/install_flag_gems 2, strip_vendor_paths 1).
  2. Found the interpreter divergence: every script but MetaX sets VENV_PYTHON; MetaX runs the image's /opt/venv via PATH and uses bare python. CUDA drives several interpreters per call.
  3. Verified no non-function code would be removed by diffing the removed non-comment lines against the shared function bodies.

Solution Design

Implementation approach:

One sourced library; each script sources it after the pin file and deletes its local copies.

Key design decisions:

  • Interpreter via ${VENV_PYTHON:-python}. Preserves MetaX's python (it sets no VENV_PYTHON) and everyone else's VENV_PYTHON.
  • pip_retry parameterized by environment, not by argument. PIP_RETRY_PYTHON (per-call interpreter override for CUDA), PIP_RETRY_TIMEOUT (default 300), PIP_RETRY_NO_CACHE (default 1). CUDA sets 600/0 once and prefixes its 7 calls with PIP_RETRY_PYTHON=… instead of passing the interpreter as $1; every other script's call sites are unchanged.
  • Rationale moved, not deleted. The FlagGems VCS-install story, the "three attempts" note and the retry rationale now live in the library next to the code they explain.
  • Scoped to helpers. The issue's unified set_env.sh --platform <p> runner (item 2) and a constraints.txt (item 3) are separate, larger changes; deferred.

Code changes by file:

  • .github/scripts/lib/set_env_common.sh (new): the five helpers + the moved rationale.
  • .github/scripts/set_env_*.sh: source the library; drop the local helpers and duplicated comments; CUDA prefixes its pip_retry calls.
  • tests/unit/test_set_env_common.py (new): the library defines the helpers, no script redefines them, every script sources it, CUDA still selects its interpreter per call.
  • .github/workflows/agnostic-checks.yml: bash -n the library; run the new test.

Changes by commit:

  1. cafd32f - ci: share the set_env_* helper functions in one library: the whole change.

Verification

Pre-submission Checklist

  • Linting passed (ruff check, ruff format --check)
  • Type checking passed (not applicable)
  • All tests pass (unit)
  • Manual testing completed (bash -n; removed-code audit)
  • No debug/temporary code
  • Documentation updated (rationale moved into the library)
  • Commit messages follow conventions
  • All text in English

Linting Results

$ ruff check
All checks passed!

$ ruff format --check
303 files already formatted

$ for f in .github/scripts/set_env_*.sh .github/scripts/lib/set_env_common.sh .github/version-pins.env; do bash -n "$f"; done
# (no output)

Test Results

# Command:
python3 -m pytest tests/unit/test_set_env_common.py tests/unit/test_ci_version_pins.py -q -p no:cacheprovider

# Output:
9 passed in 0.02s

Manual Verification

# No script keeps a local copy of a shared helper:
$ grep -cE "^pip_retry\(\)|^flag_gems_installed\(\)|^install_flag_gems\(\)|^strip_vendor_paths\(\)|^venv_is_usable\(\)" .github/scripts/set_env_*.sh
# 0 for every script

# Every script sources the library (2 = the shellcheck comment + the source line):
$ grep -c "set_env_common.sh" .github/scripts/set_env_*.sh
# 2 per script

# The duplicated rationale is gone from the scripts and present once in the lib:
$ grep -rn "FlagGems install is a VCS install\|Retries are deliberate\|Three attempts at most" .github/scripts/set_env_*.sh
# (no output)
$ grep -c "FlagGems install is a VCS install" .github/scripts/lib/set_env_common.sh
# 1

# Removed non-comment lines are all within the shared function bodies (the only
# other removals are CUDA's pip_retry call sites, rewritten in place).

The end-to-end check is the platform CI jobs, which source these scripts; the shared helpers resolve the same interpreter and flags as before.

Breaking Changes

None. Each script resolves the same interpreter and pip flags as before; only where the helper is defined changed.

Code Quality Verification

Style Consistency

  • Matched existing code style in modified files
  • Comment density matches surrounding code
  • Reused the existing ${VENV_PYTHON:-python} convention

Edge Cases Considered

  1. MetaX has no VENV_PYTHON, so ${VENV_PYTHON:-python} keeps its python.
  2. CUDA drives three interpreters, so it sets PIP_RETRY_PYTHON per call; the test pins that all 7 calls do.
  3. set -u is in effect in every script: every variable read by the library is either set by the caller or has a :- default.
  4. The moved rationale is identical in the scripts, so moving it once loses nothing.

Potential Risks

  1. A typo in a helper would surface at runtime under set -u; bash -n catches syntax, and the platform CI jobs exercise the scripts end to end.
  2. The library is sourced before the helpers are called in every script (right after the pin file), so ordering is safe.

Rollback Plan

Revert the single commit; the scripts regain their local helpers.

Related Work

Explicitly Not Included

  • The unified .github/scripts/set_env.sh --platform <p> runner with per-platform hooks (issue item 2) -- a large inversion of control over seven vendor scripts, verifiable only on the vendor runners.
  • A constraints.txt for the CI venvs (issue item 3).
  • Reconciling setup.py's flag_gems>=5.0.2 floor with CI's pinned git revision (issue item 4).

Human Review Notes

Areas needing special attention:

  1. That the shared helpers resolve the same interpreter/flags as the copies they replace (especially MetaX's python and CUDA's per-call interpreters).
  2. That no per-platform main-flow code was removed (the audit compares removed non-comment lines to the shared function bodies).

Questions for reviewer:

  1. Land the library now and do the runner refactor later, or fold them together?

Additional Context

Base: upstream/main at 877afb1.


🤖 Generated with Claude Code

Part of flagos-ai#378

The seven platform provisioning scripts each carried their own copy of
pip_retry (five slightly different spellings), flag_gems_installed and
install_flag_gems (six copies each), strip_vendor_paths (three) and
venv_is_usable (two), plus a 21-line FlagGems VCS-install rationale duplicated
verbatim in six of them. A fix had to be applied seven times and was easy to
miss.

Add .github/scripts/lib/set_env_common.sh and have every script source it,
removing the local copies and the now-duplicated comments (the shared rationale
moved into the library). The per-platform main flows, SDK discovery and
vendor-torch handling are unchanged; only the shared helpers moved.

The interpreter comes from ${VENV_PYTHON:-python}: every script but MetaX sets
VENV_PYTHON to its job-local venv, and MetaX runs the image's /opt/venv via PATH
and keeps using `python`. pip_retry additionally honours PIP_RETRY_PYTHON (the
CUDA script drives several interpreters, so it now sets that per call instead of
passing the interpreter as $1), PIP_RETRY_TIMEOUT and PIP_RETRY_NO_CACHE (CUDA
keeps its 600s timeout and build cache; the rest keep 300s + --no-cache-dir).

Changes:
- .github/scripts/lib/set_env_common.sh (new): the five helpers.
- .github/scripts/set_env_*.sh: source the library; drop the local helpers and
  their duplicated comments; CUDA prefixes its 7 pip_retry calls with
  PIP_RETRY_PYTHON.
- tests/unit/test_set_env_common.py (new): the library defines the helpers, no
  script redefines them, every script sources it, and CUDA still selects its
  interpreter per call.
- .github/workflows/agnostic-checks.yml: bash -n the library; run the new test.

Net -675/+285 across the seven scripts (the shared logic now exists once).

Tested: bash -n on all scripts and the library; ruff clean; 26 pure-text unit
tests pass; the only non-function code removed is CUDA's pip_retry call sites,
rewritten in place.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lvyufeng

Copy link
Copy Markdown
Collaborator Author

Superseded by #420, which stacked on this branch's commit (cafd32f) and merged. Everything here — the shared .github/scripts/lib/set_env_common.sh, the seven scripts sourcing it, tests/unit/test_set_env_common.py, and the agnostic-checks wiring — is already in main via #420. The CI failures on this branch (DCU one-hour timeout from the FlagGems git clone; PPU requirements-hash mismatch) were the environment-install problems #420 fixed by moving to published binary wheels, not the helper refactor. Closing as redundant; no rebase would leave a non-empty diff.

@lvyufeng lvyufeng closed this Sep 25, 2026
@lvyufeng
lvyufeng deleted the refactor/set-env-common branch September 25, 2026 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant