scale_m1: add --racks and update docs#531
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds the new scale_m1 GB200/GB300 (gbx00) scaling utility with rack-based sizing (--racks / --overprovision-racks), plus documentation and packaging/installation wiring so it ships with the azure-slurm installer.
Changes:
- Introduce
scale_m1/scale_to_n_nodes.pyCLI and supporting mock/test harnesses. - Add rack-based CLI sizing (
--racks,--overprovision-racks) and document the intended operational workflow inREADME.md. - Package/install
scale_m1viaazure-slurm/package.pyandazure-slurm/install.sh, and add scheduler upgrade scripts.
Scorecard
| Criterion | Max Points | Points Awarded | Notes |
|---|---|---|---|
| PR Description Accuracy | 20 | 0 | No PR description provided (only title). |
| PR Atomicity | 20 | 0 | Includes scale_m1 feature + two new scheduler upgrade scripts (separate concern). |
| Logical Implementation | 10 | 10 | Core approach is reasonable, but see blocking correctness issues called out in review comments. |
| Regression Risk | 10 | 0 | Critical issues in upgrade scripts and scaling behavior create high operational risk. |
| Exception Handling | 10 | 10 | Generally handled with SlurmM1Error wrappers; no new obvious silent failures beyond noted issues. |
| Code Comments | 10 | 10 | No clear new “HOW not WHY” comment problems. |
| Repetitive Code | 10 | 0 | Large test file introduces substantial repetition. |
| Spelling | 5 | 0 | New spelling/grammar issues present in added code/comments. |
| Logging Quality | 5 | 0 | Error/log output includes misleading/inverted comparison and overly noisy locals() dump. |
FINAL SCORE: 30/100
RECOMMENDATION: DO NOT MERGE
RATIONALE: The PR introduces critical correctness/operational issues (notably in the new scheduler upgrade scripts and in test/mock initialization) that can break upgrades/tests and mislead operators.
BLOCKERS:
- Fix
upgrade_scheduler*.shpython discovery/installation flow underset -eso upgrades work when python3.11 isn’t already installed. - Remove import-time global logging side effects in
scale_m1/mock.pyto prevent unit test/import failures. - Address scaling behavior mismatches (e.g., termination wait semantics and the unused
--timeoutflag).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| scale_m1/scale_to_n_nodes.py | New scale_m1 CLI implementation, including --racks support and scaling/pruning logic. |
| scale_m1/mock.py | Mock Slurm + mock topology generator used for tests and local validation. |
| scale_m1/scale_to_nodes_test.py | Larger end-to-end style tests for power-up/prune behavior using mocks. |
| scale_m1/mock_test.py | Focused unit tests for mock command parsing/state transitions. |
| scale_m1/cli_args_test.py | Unit tests covering racks/nodes CLI argument resolution and invariants. |
| README.md | New documentation section describing scale_m1 workflow, sizing, and operational guidance. |
| azure-slurm/package.py | Packages the scale_m1 runtime entrypoint into the azure-slurm tarball. |
| azure-slurm/install.sh | Installs scale_m1 into the venv and symlinks it into ~/bin. |
| azure-slurm-install/upgrade_scheduler.sh | New scheduler upgrade helper script (contains critical python discovery/install issues). |
| azure-slurm-install/upgrade_scheduler_reset.sh | New upgrade+reset helper script (contains the same critical issues). |
Comment on lines
+14
to
+19
| for version in $( seq 11 20 ); do | ||
| which python3.$version | ||
| if [ $? == 0 ]; then | ||
| return 0 | ||
| fi | ||
| done |
Comment on lines
+49
to
+54
| for version in $( seq 11 20 ); do | ||
| which python3.$version | ||
| if [ $? == 0 ]; then | ||
| return 0 | ||
| fi | ||
| done |
Comment on lines
+24
to
+28
| install_python3() { | ||
| PYTHON_BIN=$(find_python3) | ||
| if [ -z "$PYTHON_BIN" ]; then | ||
| return 0 | ||
| fi |
Comment on lines
+59
to
+63
| install_python3() { | ||
| PYTHON_BIN=$(find_python3) | ||
| if [ -z "$PYTHON_BIN" ]; then | ||
| return 0 | ||
| fi |
ryanhamel
force-pushed
the
ryhamel/scale_m1_merge
branch
from
June 25, 2026 13:52
f5581f3 to
5ef5b94
Compare
| help='Enable verbose logging') | ||
| prune_now.add_argument('--reservation', required=False, help="Optional: use an existing reservation", | ||
| default="scale_m1") | ||
| prune_now.add_argument("--termination-list", help="T") |
ryanhamel
force-pushed
the
ryhamel/scale_m1_merge
branch
from
June 25, 2026 14:02
5ef5b94 to
226d148
Compare
ryanhamel
force-pushed
the
ryhamel/scale_m1_merge
branch
from
June 25, 2026 16:43
226d148 to
5001481
Compare
- Add --racks / --overprovision-racks (1 rack = NODES_PER_RACK = 18 nodes) to power_up/prune/prune_now, mutually exclusive with the node-count flags. - Make -b/--overprovision optional (defaults to 0). - Centralize target/overprovision resolution and extract build_parser(); add unit tests in cli_args_test.py. - Package scale_to_n_nodes.py in azure-slurm (mock.py excluded) and install it scheduler-only via install.sh, mirroring the azslurm wrapper+symlink. - Document scale_m1 in the README. Behavior change: power_up no longer requires -b. scale_m1: document deleting the reservation after scaling Once nodes are powered up and pruned, the scale_m1 reservation must be deleted (scontrol delete reservation scale_m1) to release the nodes back to the cluster so workloads can be scheduled on them. scale_m1: add --mock to prune, registered only when mock is importable prune gains a --mock flag (dest mock_topology) that uses the mock topology instead of querying the cluster. It is registered only when the test-only mock module is importable (checked via importlib find_spec, so mock.py's import-time side effects are not triggered), keeping the option hidden in the installed package where mock.py is not shipped. docs: document scale_m1 GB200/GB300 workflow with block topology
ryanhamel
force-pushed
the
ryhamel/scale_m1_merge
branch
from
June 25, 2026 16:49
5001481 to
9429194
Compare
aditigaur4
approved these changes
Jun 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.