feat: migrate to modern PyPI packaging - #119
Open
coketaste wants to merge 20 commits into
Open
Conversation
Replace legacy setup.py with pyproject.toml-only build using hatchling and versioningit. Add CI workflow (tests + lint on Python 3.9-3.12), publish workflow (trusted publishing to TestPyPI/PyPI), MANIFEST.in for sdist, and PEP 561 py.typed marker. Pin dependency lower bounds and update Python baseline from 3.8 to 3.9. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
coketaste
requested review from
Cemberk,
Rohan138,
gargrahul and
leconcio
as code owners
May 1, 2026 17:29
There was a problem hiding this comment.
Pull request overview
Migrates madengine packaging and release automation to a modern, pyproject.toml-driven build using Hatchling + Versioningit, adds CI/publish workflows, and updates project metadata (including Python >=3.9 and dependency lower bounds).
Changes:
- Remove legacy
setup.pyand rely on Hatchling (pyproject.toml) with dynamic git-tag-based versioning viaversioningit. - Add GitHub Actions workflows for CI (tests + lint on 3.9–3.12) and publishing to (Test)PyPI using trusted publishing.
- Add packaging metadata/files (
py.typed,MANIFEST.in, updated classifiers/keywords,.gitignoredist artifacts).
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
setup.py |
Removed legacy setuptools-based build/compat layer. |
pyproject.toml |
Updates metadata (Python>=3.9, deps lower bounds), dynamic versioning config, and Hatch build configuration. |
src/madengine/py.typed |
Adds PEP 561 marker for downstream type checkers. |
MANIFEST.in |
Adds sdist inclusion rules (note: may not be used by Hatchling). |
.github/workflows/ci.yml |
New CI workflow running tests and linting. |
.github/workflows/publish.yml |
New build/test-install/publish workflow for (Test)PyPI. |
.gitignore |
Ignores wheel/tarball outputs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add fetch-depth: 0 to lint job so versioningit can derive version - Create .flake8 config with max-line-length=88 matching black - Add package asset verification step to publish workflow - Ignore pre-existing lint categories (E501, F541, etc.) that need separate cleanup Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Hatchling ignores MANIFEST.in — sdist/wheel inclusion is configured via [tool.hatch.build] in pyproject.toml. Keeping it creates false confidence about what gets packaged. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
- Apply black formatting across all src/ and tests/ files - Sort imports with isort (profile=black) - Remove unused imports (F401): os, sys, Path, argparse, Context, GPUInstallationError, validate_rocm_installation - Remove unused variables (F841): exception aliases, manifest_file, result, BASE_DIR, gpu_info, job, pull_error - Fix shadowed import (F402): rename loop var 'field' to 'field_name' in mongodb.py Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
E2e tests run `madengine run` which needs Docker and GPU access, neither available on GitHub Actions runners. Exclude tests/e2e/ from CI to avoid 50 false failures. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
… e2e tests pytest.ini defined testpaths including tests/e2e/, which took precedence over pyproject.toml and caused e2e tests to run in CI despite --ignore. Consolidate all pytest config into pyproject.toml with the correct markers, fix python_paths → pythonpath, and add --strict-markers. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Typer/Rich injects ANSI escape sequences into --help output that can split option names mid-word on CI runners, causing string-in-string assertions to fail despite the flag being present. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
coketaste
changed the title
Resolves 18 conflicted files. Resolution policy: take develop's functional changes, keep HEAD's black/isort formatting where the two only differed cosmetically. Notable resolutions: - pyproject.toml: keep [tool.hatch.build] with develop's scripts-only artifacts (templates are VCS-tracked, so hatchling includes them by default); keep testpaths=["tests"] plus develop's minversion and marker set. - kubernetes.py, container_runner.py: take develop wholesale — HEAD's only change was the e0ff8cc formatting pass, while develop decomposed kubernetes.py into k8s_{pvc,results,scripts, template_context}.py mixins. - docker_builder.py: use develop's non-parenthesized context managers (HEAD's form requires Python 3.10; the project targets 3.9+). - conftest.py: drop the duplicate pytest_configure marker registration — markers now live in pyproject.toml. - CHANGELOG.md: develop's version numbering is authoritative (its 2.1.0 is 2026-05-28 and 2.1.1-2.1.3 followed); HEAD's still-accurate PyPI packaging notes moved to [Unreleased], with the MANIFEST.in entry updated to reflect its later removal. Verified: no conflict markers remain, all merged modules import, and 741 unit tests pass. CI lint (black/isort/flake8) fails on code merged in from develop; fixed in a follow-up style commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The develop merge brought in code predating this branch's formatting pass (e0ff8cc), so the CI lint job failed. Reapplies black and isort across src/ and tests/, and fixes the four flake8 findings the formatters cannot: - kubernetes.py: drop unused 'job' assignment from create_namespaced_job - container_runner.py: drop unused 'e' / 'pull_error' exception aliases - build_orchestrator.py: remove the redundant local 'import os' in _execute_build_on_compute that shadowed the module-level import and left it flagged as unused black --check, isort --check, and flake8 all pass; 741 unit tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ersions The merge of develop into this branch took develop's dependency layout, which removed the [dev]/[kubernetes]/[all] extras in 2.1.1 and folded those packages into base `dependencies`, but kept this branch's ci.yml still referencing `.[dev]`. Older pip silently ignores the unknown extra; newer pip warns. Use a plain editable install — the dev toolchain (pytest, black, isort, flake8, mypy, pre-commit) now installs unconditionally. Also collapse a parenthesized import in test_errors.py to a single line. CI installs isort unpinned and gets 9.x, which formats this import differently than the locally installed 8.x and failed `isort --check`. The single-line form is what both 8.x and 9.x emit, so it is stable across the version drift rather than pinned to one release. Verified in a fresh clone and fresh venv: `pip install -e .` succeeds, 741 passed / 1 skipped, and black/isort/flake8 exit 0 under both the newest toolchain and the older local one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The v2.0.0 "Installation & Setup" block said "Python: 3.9+ (3.8 support dropped in v2.0.2)". Both halves are wrong. v2.0.2 was a credential.json type-validation fix and dropped nothing; develop still declares requires-python = ">=3.8" today. The line was rewritten on this branch in 81eed01 when the packaging work was headed for a 2.1.0 heading, and it stayed put after the work moved under [Unreleased]. Restore the original text, which correctly describes what v2.0.0 shipped. The 3.8 -> 3.9 bump is already recorded under [Unreleased], where the change actually lands. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 137 out of 139 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
CHANGELOG.md:28
- PR description calls out adding
MANIFEST.in, but the changelog saysMANIFEST.inwas removed. Since this PR’s current file set doesn’t include aMANIFEST.in, this entry is confusing and should be reconciled (either drop theMANIFEST.inbullet here or update the PR description to match the final approach of configuring inclusion via[tool.hatch.build]).
* fix(run_orchestrator): preserve multiple_results in local-image manifest (#166) MAD_CONTAINER_IMAGE (local image) mode built a synthetic manifest that omitted the models.json `multiple_results` field. Without it, ContainerRunner never sets MAD_OUTPUT_CSV, never copies the perf CSV out of the container, and falls back to scraping the run log for a "performance: NUMBER METRIC" line -- reporting FAILURE even when the model produced valid perf-CSV results. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> * feat(auth): reuse existing docker login (OAT) instead of requiring credential.json (#168) * feat(auth): reuse existing docker login instead of requiring credential.json Reads ${DOCKER_CONFIG:-~/.docker}/config.json the same way the Docker CLI does, so a machine already authenticated via `docker login` (including an org access token) is not forced to duplicate credentials into credential.json, and blank placeholder credentials never override or break a working login. Also distinguishes insufficient_scope (authorization) from unauthorized (authentication) in base-image pull failures so the error message points at the right fix, and logs in before `docker build --pull` only when there's no existing login to reuse. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(gpu-tools): support non-default ROCm install paths and detect GPUs via PATH Honor $ROCM_PATH (falling back to /opt/rocm) instead of hardcoding /opt/rocm in amd_smi_utils.py, rocm_smi_utils.py, and gpu_info_profiler.py, and detect nvidia-smi/rocm-smi/amd-smi via `command -v` in gpu_info_pre.sh instead of a fixed binary path, so detection works when ROCm is installed elsewhere or GPU tools are only on PATH. Also make the rpd tracer's LD_LIBRARY_PATH ROCm-path-aware, and fall back to saving the raw trace.rpd when rpd2tracing.py fails instead of losing the trace. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(gpu-tools): guard rocminfo call and detect nvidia-smi/rocm-smi via PATH rocminfo may be absent even when rocm-smi/amd-smi are present; call it only when available instead of failing the pre-script. Also prefer PATH lookups over hardcoded /usr/bin and $ROCM_PATH/bin paths when detecting GPU vendor. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(auth): name the actual registry in pull-denied hints explain_registry_denial() always suggested Docker Hub credentials even when the failing image referenced another registry (e.g. ghcr.io). Extract the registry host from the image reference and tailor the docker login / credential.json suggestions to it, falling back to the existing Docker Hub guidance when the image has no registry host. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> * fix(slurm): make the generated job script portable across clusters (#163) * feat(slurm): allow opting out of the --gpus-per-node sbatch directive Clusters that do not advertise GPU GRES reject any job script carrying --gpus-per-node, so the generated sbatch fails before launch. Add slurm.skip_gpus_directive (default false) to omit the directive and rely on exclusive/nproc_per_node instead. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(slurm): raise the madengine availability probe timeout The pre-submission check ran `madengine --version` with a 5s timeout, which a cold interpreter start off shared/NFS storage exceeds, aborting submission on a perfectly healthy environment. Raise it so the probe only catches a hang. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(run): cap the informational rocm-libs package query The node-info step shelled out to the host package manager with no time limit. On a node where yum wants to import a repo GPG key the command waits on a prompt that never arrives, so the whole multi-node run hangs before the workload starts. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(slurm): inherit the submitter's PATH in the sbatch job A batch job is not guaranteed to inherit the submitter's PATH: a site can default sbatch to --export=NONE, and the module loads in the job body can rewrite it. The pre-submission check then passes on the login node while the compute node aborts with "madengine not found in PATH". Render the per-user bin directory and the directory the madengine console script was resolved from at submission time into the generated script, so the job puts the same interpreter back on PATH instead of relying on inheritance. * fix(slurm): match nfs4 in the shared-filesystem probe The single-node workspace probe matched \bnfs\b only, but df -T reports nfs4 on most modern NFS mounts. A shared submission directory was therefore classified as node-local and the job copied the whole project into /tmp instead of using the shared path. Match \bnfs[0-9]*\b so nfs, nfs3 and nfs4 are all recognized. The rendered job script now also has coverage for the --gpus-per-node opt-out it grew earlier in this batch: skip_gpus_directive shipped without tests, so nothing failed if the directive crept back into the template. Both states of the flag are asserted against the rendered script. * fix(slurm): read the filesystem type, not the whole df line The shared-filesystem probe grepped the entire `df -T` output line, which carries the mount point as well as the type. A local disk mounted at a path such as /mnt/nfs-scratch therefore matched, the submission directory was classified as shared, and the single-node job worked out of storage the other side of the run could not see. Read the type column alone via `df --output=fstype` and anchor the pattern to it. The option is GNU coreutils 8.21 and up, so an awk fallback over `df -T` covers older systems. beegfs and panfs join the list of shared types while the pattern is being rewritten; both are common enough on HPC sites to be worth recognizing. --------- Co-authored-by: Mikhail Kuznetsov <mkuznets@ruby-slurmlogin01.rckg.g03.cpe.ice.amd.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Stephen Shao <yu.shao@amd.com> * docs: rewrite README as concise landing page and add architecture diagrams (#161) * docs(readme): rewrite README as concise landing page with Mermaid figures Trim root README from 707 to 258 lines by moving deep reference material (profiling tables, extended config/usage recipes, tips) into docs/ and linking out. Replace the ASCII architecture block and the stale, unreferenced docs/img PNGs with accurate inline Mermaid figures for the layered architecture, build->run->report pipeline, and deployment-target inference. Move the parallelism/infrastructure capability matrices into docs/launchers.md and add Mermaid workflow diagrams to docs/deployment.md and docs/README.md. Co-Authored-By: Claude <noreply@anthropic.com> * docs: fix CLI flag names and defaults across README, cli-reference, usage Corrects --csv-file to the real --csv-file-path/--file flags, fills in missing `database` command flags (--unique-key/-k, --batch-size, --no-upsert, --no-index, --dry-run, MONGO_AUTH_SOURCE/MONGO_TIMEOUT_MS), fixes wrong `run --output`/`--tools-config` defaults, fixes broken space-separated --tags syntax, and updates the stale version badge. * docs(configuration): remove fabricated keys, fix defaults and schemas Removes fabricated timeout_multiplier/service_account keys and the vllm.tensor_parallel_size example (never read; real value derives from distributed.nproc_per_node). Fixes SLURM gpus_per_node default (1 -> 8), docker_env_vars.MAD_ROCM_PATH -> ROCM_PATH, the Data Provider schema (fabricated data_sources wrapper -> real flat shape), and credential key names (AMD_GITHUB -> PUBLIC_GITHUB_ROCM_KEY, uppercase USERNAME/PASSWORD). * docs(deployment): fix config priority order and stale example references Corrects the backwards/incomplete K8s "Configuration Priority" list to match config_loader.py's real 5-layer merge order (including the previously-missing AMD multi-GPU vendor layer), and removes nonexistent SLURM fields (mem, mail_user, mail_type). In examples/k8s-configs and examples/slurm-configs READMEs: fixes ~35 stale/renamed file paths, removes dead Primus example references, marks gpu_vendor/guest_os/deploy as non-required with real defaults, removes the invalid "RHEL" guest_os value, documents results_pvc and host_ipc as non-configurable (host_ipc previously told users to set a key that is always silently overridden), adds missing results_storage_size / data_storage_size / allow_privileged_profiling fields, and documents the SLURM node-health preflight keys (enable_node_check, auto_cleanup_nodes, allow_submit_without_clean_nodes, verbose_node_check). * docs(launchers): fix launcher value, dead config keys, and wrong examples Fixes "megatron" -> "megatron-lm" launcher value, removes the nonexistent --config flag in favor of --additional-context-file, clarifies the dead distributed.master_port key (SLURM reads distributed.port; K8s reads a separate top-level launcher.master_port object), corrects the 5-node auto-split table row (1/3, not 2/2), fixes the vLLM SLURM multi-node description (data-parallel, not TP+PP with Ray), and removes broken Primus example links. * docs(profiling): fix tool flags, env var names, and default config example Removes the nonexistent --tools therock_check flag, fixes the "Default Tool Configuration" example (rocprof command, gpu_info_power_profiler env vars using bare names instead of the real POWER_/VRAM_-prefixed names) across all Multi-GPU and sampling-rate examples, and adds the undocumented tool names (rocprof_hip_only, rocprof_sys, rocprofv3, rocprofv3_agent, rocprofv3_agent_counter, hipblaslt_trace, instruction_mix.txt). * docs: fix batch-build manifest example and stale install/contributing refs Notes that deployment_config in build_manifest.json is only written for non-local deployments (per _save_deployment_config in build_orchestrator.py), and adds the always-present context/ credentials_required keys to the example. Removes a duplicated `madengine --version` line in installation.md and fixes a stale test path in contributing.md (tests/test_cli.py -> tests/unit/test_cli.py). * docs(database): rewrite README to describe the shipped mongodb module The README described the module as "Not yet implemented" and documented a fictional future API (mongodb_client.py/MongoDBClient, local_storage.py/LocalStorage, api.py/ingest_results()), even though mongodb.py is fully implemented and wired into the `database` CLI command. Rewrites the README around the real classes (MongoDBConfig, UploadOptions, UploadResult, DocumentLoader/JSONLoader/CSVLoader, DocumentTransformer, MongoDBUploader, upload_file_to_mongodb) and adds a CLI-flag-to-API-param mapping table. * docs: fix execution and reporting README signature and API mismatches execution/README.md: fixes build_all_models's models_list -> models param, run_container's fabricated model_docker/gpu_ids params -> real docker_image string param, wrong status value casing/set (successful/failed/timeout -> SUCCESS/FAILURE/SKIPPED), wrong result key (duration -> test_duration), and documents the previously-missing dockerfile_utils.py and container_runner_helpers.py files. reporting/README.md: removes the fabricated "Legacy Reporting Tools" section claiming csv_to_html.py/csv_to_email.py live in a nonexistent tools/ directory and are unused by the modern CLI (they live in reporting/ and back `report to-html`/`report to-email`), fixes the update_perf_csv()/flatten_tags() example signatures, documents the perf_entry.csv/.json side effect, and adds a missing entry for update_perf_super.py. * docs: correct launcher names, docker-login env vars, and config key references - Fix remaining megatron -> megatron-lm launcher references in README, usage, and configuration docs. - Document DOCKER_CONFIG and MAD_SKIP_DOCKER_LOGIN, and clarify that MAD_CONTAINER_IMAGE is an --additional-context key, not an env var (cli-reference, configuration, usage). - Add missing Kubernetes and SLURM additional_context keys (cluster/scheduling, storage, node health/results) with pointers to the example READMEs for full reference. - Correct SGLang Disaggregated minimum node counts and split formula to reflect the SLURM co-located-proxy layout vs. Kubernetes' dedicated proxy requirement. - Replace stale hardcoded version/date footer with a pointer to `madengine --version` and CHANGELOG.md. --------- Co-authored-by: Claude <noreply@anthropic.com> * docs: add design spec for pinned image digest enforcement Proposes capturing the pushed image digest at build time (always on) and gating enforcement of digest-pinned pulls behind an opt-in --require-pinned-image flag, addressing a run that pulled a different image than the one the build pushed due to a mutable-tag race. * docs: add implementation plan for pinned image digest enforcement Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(image-digest): add digest parsing, pinned reference, and enforcement helpers Add madengine/core/image_digest.py with four pure helpers: - parse_push_digest: extract sha256 digest from `docker push` output - parse_repo_digest: extract digest from a repo@sha256:... reference - build_pinned_reference: build repo@sha256:... , stripping any tag/digest - resolve_pinned_image: pass through, pin, or raise ConfigurationError Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(build): capture pushed image digest during docker push push_image() now captures the push output and records the resulting sha256 digest in self.pushed_digests, falling back to `docker image inspect --format '{{index .RepoDigests 0}}'` when the registry does not print a digest line. Best-effort: a missing digest is noted at dim level and never fails the build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(build): record image_digest in build manifest entries Both push call sites (single-arch and per-GPU-arch) copy the digest recorded by push_image into build_info["image_digest"]. build_info is serialized wholesale into build_manifest.json, so the key is purely additive for existing manifest consumers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(run): add --require-pinned-image flag and context propagation The CLI flag and the require_pinned_image additional-context key are equivalent entry points. The key is also persisted into manifest["context"] so the nested `madengine run` that SLURM job scripts execute on each compute node inherits the setting. Two pre-existing tests built args as a bare MagicMock and asserted exact-equality on additional_context; auto-vivified attributes are truthy, so the new flag leaked in. Pin the attribute in those mocks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(run): pin local docker pulls to manifest digest when required The resolve_pinned_image call sits outside the pull try/except so a missing digest aborts rather than falling back to the local image tag. The container runs the same pinned reference that was pulled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(k8s): pin pod image to manifest digest when required The pod spec image field resolves through resolve_pinned_image, so a moved tag surfaces as an ImagePullBackOff rather than a silent wrong-image run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(slurm): pin slurm_multi image to manifest digest when required slurm_multi runs the model's own script on the head node with no nested `madengine run` on the compute nodes, so enforcement happens here. Pinning DOCKER_IMAGE_NAME covers both the parallel `srun docker pull` (which interpolates it) and the `docker run` inside the model script. prepare()'s launcher peek wrapped the whole slurm_multi dispatch in a bare `except Exception: pass`, which would have swallowed the enforcement error and generated an unpinned script instead. Re-raise ConfigurationError so deliberate aborts propagate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(execution): cover log naming for digest-pinned image references Characterization tests: _docker_image_ref_for_log_naming already strips @sha256:..., so pinned references produce the same log/tar filenames as tags. Locks that in against future refactors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: document --require-pinned-image and image digest capture Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(run): enforce pinned digest on build-on-compute-node manifests The build-on-compute-node path writes built_images entries carrying both a truthy local_image and a registry reference in docker_image. That branch runs before the registry branch in run_models_from_manifest, so --require-pinned-image was silently a no-op for those manifests -- the exact bypass the flag exists to prevent, and contrary to the documented fail-fast behaviour. Resolve the pin in the local_image branch too. resolve_pinned_image now passes through references that are already digest-pinned, so an explicitly pinned MAD_CONTAINER_IMAGE is accepted rather than rejected for lacking a manifest digest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(execution): derive Docker-legal container name from pinned image refs Container names were built straight from the run image reference, mapping only "/" and ":" to "_". Under --require-pinned-image the run image becomes repo@sha256:..., so the "@" survived into --name and the daemon rejected it: docker: Error response from daemon: Invalid container name (container_***_mad-private@sha256_af99a16c...), only [a-zA-Z0-9][a-zA-Z0-9_.-] are allowed The pull succeeded because a digest reference is a valid image ref; only the name derived from it was invalid. Add container_name_from_image_ref(), which strips the digest, applies the same "/" and ":" mapping as before, and sanitizes any remaining out-of-charset byte so the invariant holds instead of failing at `docker run`. _docker_image_ref_for_log_naming() is deliberately not reused: it collapses CI-style refs to the bare tag, which would rename containers on every existing non-pinned path. Keeping the tag also keeps different tags of one repo distinct. Verified byte-identical output against the previous expression for 60 non-digest refs (bare tags, registry hosts, localhost:5000 ports, CI tags, dotted tags), including the name the e2e suite asserts. Also drops the dead re.sub(".*:", "", ...) at the call site: ":" had already been replaced with "_" on the preceding call, so it never matched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(run): fail pinned pulls instead of falling back to a local tag Under --require-pinned-image a failed registry pull fell back to run_image = image_name. Local tags are mutable too, so that fallback broke the pinned-image guarantee in the failure mode it matters most in: a digest/tag mismatch or an auth error would silently run whatever the local tag happened to point at. The pull failure is now fatal for that model. The surrounding handler already records it as a failed run and continues with the rest of the manifest, so one unpinnable model does not abort the whole run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(build): run docker push without the 60s command timeout push_image() called Console.sh() with the default 60s timeout, so pushing a multi-GB image failed an otherwise successful build with "Console script timeout". build_image() already passes timeout=None for docker build; do the same for the push. Console.sh's timeout was annotated int while five call sites already pass None (which subprocess accepts as "wait indefinitely"), so widen the annotation to Optional[int] to match the behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: mkuznet1 <Mikhail.Kuznetsov@amd.com> Co-authored-by: Mikhail Kuznetsov <mkuznets@ruby-slurmlogin01.rckg.g03.cpe.ice.amd.com> Co-authored-by: Cursor <cursoragent@cursor.com>
This reverts commit 6c966a8. PR #178 targeted main by mistake; it should have gone to develop. Because the head branch was based on develop while main was five PRs behind, the squash merge pulled unreleased develop work into main along with the feature: #166, #168, #163, #161 and #175. Reverting restores main to ec4de0b exactly. The feature is being re-opened against develop; no revert-of-this-revert is needed, since the squash commit shares no SHAs with develop's history and a later develop -> main merge applies cleanly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The blanket *.json .gitignore rule (for user models.json etc.) was
silently excluding src/madengine/deployment/presets/**, which hatchling
respects when building. A PyPI install therefore shipped SLURM/K8s
deployments with no defaults, gpu-vendor, or single-node/multi-node
profile presets — ConfigLoader.load_preset() returns {} on a missing
file with no error, so runs silently lost partition names, NCCL env
vars, and distributed backend config instead of failing loudly.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 137 out of 139 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
CHANGELOG.md:29
- The changelog states
MANIFEST.inwas removed, but the PR description claimsMANIFEST.inwas added. The repo state matches the changelog (noMANIFEST.inpresent), so the PR description appears stale/misleading for reviewers and release notes; please update the PR description to match the actual changes (or re-add the file if that’s truly required).
# Conflicts: # src/madengine/deployment/k8s_template_context.py # src/madengine/deployment/slurm.py # src/madengine/execution/container_runner.py # src/madengine/execution/docker_builder.py # src/madengine/orchestration/run_orchestrator.py # tests/unit/test_orchestration.py
There was a problem hiding this comment.
🟡 Changes recommended
The publish workflow’s “Verify package assets” step will fail due to an indented multi-line python -c string causing IndentationError.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 137/139 changed files
- Comments generated: 1
- Review effort level: Lite
The CI workflow added on this branch is the first to lint this code, and the tests merged in from develop fail `black --check` and `isort --check`: - test_docker_builder.py, test_container_runner.py: multi-context `with` statements reformatted to black's parenthesized form - test_slurm_multi.py: import order Formatting only; no test behavior changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
_template_context constructs a real KubernetesDeployment, whose __init__
calls k8s_config.load_kube_config() and builds API clients. GitHub runners
have no ~/.kube/config, so all three tests failed with:
RuntimeError: Failed to load Kubernetes config: Invalid kube-config
file. No configuration found.
They passed locally only because a developer kubeconfig happened to exist.
Stub both loaders and the two API client constructors so the tests still
exercise the real _prepare_template_context without needing a cluster.
Also fixes import order in the same file (isort).
Verified with HOME=/tmp/nohome KUBECONFIG=/nonexistent pytest.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Without fail-fast: false, a single Python version's failure cancels the other three mid-run, hiding whether they would have passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
src/madengine/cli/validators.py has an unsorted from .constants import ... member list that can trip the new isort --check gate in CI.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/madengine/cli/validators.py:24
- Import members in this
from .constants import ...statement are not sorted, which can causeisort --checkto fail (the repo generally keeps imported names alphabetized; e.g.cli/commands/build.pyusesDEFAULT_MANIFEST_FILE, ExitCode). Reorder the imported names here to match isort expectations.
- Files reviewed: 137/139 changed files
- Comments generated: 0 new
- Review effort level: Lite
Collaborator
Author
Copilot's suggestion (treat it as one flat alphabetical list, e.g. ExitCode, VALID_GPU_VENDORS, VALID_GUEST_OS) would actually break isort's check under this repo's config, since it ignores the type-grouping. This is a false positive — no change needed. |
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.
Summary
setup.py(307 lines) withpyproject.toml-only build usinghatchling+versioningitpublish workflow (trusted publishing)
core dependencies
Changes
Packaging
setup.pyremoved — all configuration lives inpyproject.tomlversioningit: version isderived from git tags automatically; no hardcoded strings
py.typedadded (PEP 561) — enables type checkersupport for downstream consumers
MANIFEST.inadded — ensures scripts, templates,presets, LICENSE, and README are included in sdist
pandas>=1.3,sqlalchemy>=1.4) for better reproducibilityCI/CD
.github/workflows/ci.yml— runspytest -m "not slow and not integration"and lint (black, isort, flake8) acrossPython 3.9/3.10/3.11/3.12 on every push/PR to
develop/main.github/workflows/publish.yml— builds sdist + wheel,verifies install and
madengine --helpacross all Pythonversions, then publishes via PyPI trusted publishing (no
stored API tokens needed); manual dispatch to TestPyPI for
pre-release validation
Test plan
python -m buildproduces valid sdist and wheelpy.typedpip install dist/*.whl && madengine --helpworksimport madengine; print(madengine.__version__)returnsgit-tag-derived version
pytest -m "not slow and not integration")