Skip to content

feat: add optional Slurm package - #856

Open
andreatnvidia wants to merge 2 commits into
mainfrom
andreatnvidia/feat/slurm-package
Open

feat: add optional Slurm package#856
andreatnvidia wants to merge 2 commits into
mainfrom
andreatnvidia/feat/slurm-package

Conversation

@andreatnvidia

Copy link
Copy Markdown
Contributor

📋 Summary

Adds the optional data-designer-slurm distribution and exposes it through the exact-version data-designer[slurm] extra. This establishes package isolation and shared-namespace behavior before Slurm runtime features are added.

🔗 Related Issue

Closes #852

Part of #850

🔄 Changes

  • Add data-designer-slurm as a publishable workspace package under data_designer.slurm.
  • Generate exact same-version dependencies in both directions while keeping the leaf optional for base installs.
  • Extend dependency auditing to evaluate explicitly selected static and dynamic extras.
  • Add clean built-wheel checks for base-only, extra-selected, direct-leaf, missing-counterpart, metadata, and namespace behavior.
  • Wire the package into development, test, lint, build, and CI workflows.

🧪 Testing

  • .venv/bin/pytest packages/data-designer/tests packages/data-designer-slurm/tests - 1121 passed, 1 skipped
  • make test-slurm-wheel-install
  • .venv/bin/ruff check --fix .
  • .venv/bin/ruff format .
  • .venv/bin/python scripts/update_license_headers.py --check
  • uv lock --check
  • Unit tests added/updated
  • Built-wheel installation tests added

✅ Checklist

  • Follows commit message conventions
  • Commits are signed off (DCO)
  • Architecture docs updated

Add the shared-namespace leaf package and publish it through the same-version data-designer[slurm] extra. Cover resolver, namespace, base-only isolation, and built-wheel installation behavior.\n\nCloses #852

Signed-off-by: Andre Manoel <amanoel@nvidia.com>
Signed-off-by: Andre Manoel <amanoel@nvidia.com>
@andreatnvidia
andreatnvidia marked this pull request as ready for review August 10, 2026 19:14
@andreatnvidia
andreatnvidia requested a review from a team as a code owner August 10, 2026 19:14
@greptile-apps

greptile-apps Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds an optional data-designer-slurm workspace distribution and exposes it through the exact-version data-designer[slurm] extra.

  • Adds the shared-namespace Slurm package, metadata, lockfile entries, and package tests.
  • Extends dependency auditing to account for selected static and dynamically generated extras.
  • Adds clean-wheel installation tests and integrates the package into development and CI targets.
  • Leaves the synchronized publishing workflow unable to upload the new leaf distribution.

Confidence Score: 4/5

The PR should not merge until the synchronized release workflow uploads data-designer-slurm, otherwise the newly advertised Slurm extra will be uninstallable from PyPI.

The package metadata introduces an exact dependency on a new leaf distribution, but the sole repository publishing path still uploads only the original three packages.

Files Needing Attention: packages/data-designer/pyproject.toml and scripts/publish.sh

Important Files Changed

Filename Overview
packages/data-designer/pyproject.toml Adds the exact-version Slurm extra, but the required leaf artifact is omitted from the synchronized publishing path.
packages/data-designer-slurm/pyproject.toml Defines the new leaf distribution and its exact matching dependency on the base package.
scripts/audit_package_dependencies.py Extends dependency inventory generation to include explicitly selected static and dynamic extras.
scripts/test_slurm_package_install.py Thoroughly checks locally built wheel metadata, base and extra installation behavior, direct-leaf installation, and missing-counterpart failure.
Makefile Integrates the Slurm package into tests, formatting, linting, builds, imports, and cleanup, but building its wheel alone does not publish it.
.github/workflows/ci.yml Adds the Slurm package and built-wheel checks to required CI test aggregation.
uv.lock Records the new workspace member and optional exact-version dependency edge without changing flagged third-party dependency versions.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Release["make publish VERSION=X"] --> Build["Build four wheels"]
  Build --> Upload["publish.sh PACKAGE_DIRS"]
  Upload --> Config["data-designer-config X"]
  Upload --> Engine["data-designer-engine X"]
  Upload --> Base["data-designer X"]
  Base -->|"slurm extra requires exact X"| Missing["data-designer-slurm X not uploaded"]
  Missing --> Failure["Extra installation fails"]
Loading
Prompt To Fix All With AI
### Issue 1
packages/data-designer/pyproject.toml:59
**Slurm release artifact is omitted**

When a maintainer releases version X through `make publish`, this extra requires `data-designer-slurm==X`, but the publishing script uploads only the original three distributions, causing `pip install data-designer[slurm]==X` from PyPI to fail dependency resolution.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "fix: address Slurm package review findin..." | Re-trigger Greptile

"rich>=13.7.1,<15",
"typer>=0.12.0,<1",
]
optional-dependencies = { slurm = ["data-designer-slurm=={{ version }}"] }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Slurm release artifact is omitted

When a maintainer releases version X through make publish, this extra requires data-designer-slurm==X, but the publishing script uploads only the original three distributions, causing pip install data-designer[slurm]==X from PyPI to fail dependency resolution.

Knowledge Base Used: Testing and Tooling

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/data-designer/pyproject.toml
Line: 59

Comment:
**Slurm release artifact is omitted**

When a maintainer releases version X through `make publish`, this extra requires `data-designer-slurm==X`, but the publishing script uploads only the original three distributions, causing `pip install data-designer[slurm]==X` from PyPI to fail dependency resolution.

**Knowledge Base Used:** [Testing and Tooling](https://app.greptile.com/nvidia-public-github/-/custom-context/knowledge-base/nvidia-nemo/datadesigner/-/docs/testing-and-tooling.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

@github-actions

Copy link
Copy Markdown
Contributor

Thanks for putting this together, @andreatnvidia — this is a clean, well-scoped foundation for the Slurm work.

Summary

This PR adds the optional data-designer-slurm workspace package (namespace data_designer.slurm) and exposes it via the exact-version data-designer[slurm] extra, establishing package isolation and shared-namespace behavior ahead of any Slurm runtime features. It wires the new package into the build/test/lint/CI tooling and extends the dependency auditor to evaluate selected extras. The implementation matches the stated intent: the package is intentionally a near-empty shell, with the substance being packaging correctness, which is validated by an impressively thorough built-wheel test.

Findings

Warnings — Worth addressing

scripts/publish.sh:31 — New package is not wired into the publish pipeline

  • What: PACKAGE_DIRS still lists only data-designer-config, data-designer-engine, and data-designer. data-designer-slurm is added to install, build, test, lint, and CI, but not to the publisher. The post-publish summary URLs (publish.sh:544-546, 564-566) are also hardcoded to the three existing packages.
  • Why: The base data-designer wheel now declares data-designer-slurm=={{ version }} under the slurm extra, and the README advertises pip install "data-designer[slurm]". Once a release goes out, data-designer[slurm] will resolve to a data-designer-slurm version that was never uploaded to PyPI, so the extra install will fail for real users. The excellent missing-counterpart case in test_slurm_package_install.py:799-819 actually demonstrates exactly this failure mode.
  • Suggestion: Add packages/data-designer-slurm to PACKAGE_DIRS (and a corresponding project URL in both summary blocks). If publishing is intentionally deferred to a later PR in the Epic: Slurm batch execution v1 #850 series, a one-line note in the PR description or a # TODO(#850): publish slurm marker would make that explicit so it isn't forgotten at release time.

Suggestions — Take it or leave it

Makefile:209test-slurm uses .venv/bin/pytest while sibling targets use uv run --group dev

  • What: test-config/test-engine/test-interface all invoke uv run --group dev pytest ..., which auto-syncs the environment. The new test-slurm (and test-slurm-wheel-install, verify-imports, format-slurm, etc.) shell out to .venv/bin/... directly.
  • Why: make test now depends on test-slurm, so running make test on a machine without a populated .venv would fail on the Slurm leg where the other legs would have self-provisioned. It's a minor consistency wrinkle rather than a bug (CI runs make install-dev first).
  • Suggestion: Consider uv run --group dev pytest $(SLURM_TESTS) for test-slurm to match the neighbors, unless the .venv/bin form is a deliberate choice worth a short comment.

Makefile:220test-isolated doesn't cover the Slurm package

  • What: The isolated-test suite (test-isolated → config/engine/interface) verifies dependency boundaries per package but omits slurm.
  • Why: The new package's whole reason to exist is its packaging/dependency boundary, and the built-wheel test already covers isolation well — so this is low priority. But an isolated target would keep the pattern uniform if the package later gains code.
  • Suggestion: Optional — add test-slurm-isolated if/when the package has importable logic worth isolating. Fine to leave as-is for now given the wheel test.

What Looks Good

  • scripts/test_slurm_package_install.py is genuinely excellent — it validates version pinning in both directions, the extra marker, Provides-Extra, the absence of a top-level __init__.py in the leaf wheel, and all four install permutations (base-only, extra, direct-leaf, missing-counterpart). That last case proving the install fails without the counterpart is exactly the right kind of negative test.
  • The audit extension (audit_package_dependencies.py) is a clean, backward-compatible addition: selected_extras defaults to None, the PACKAGE:EXTRA CLI parsing validates its format, and both static and dynamic optional-dependency sources are handled symmetrically. The accompanying test_includes_selected_dynamic_optional_dependencies test exercises the real dynamic-hook path.
  • Docs and invariants stay honest: AGENTS.md is updated to four packages and, nicely, reframes the layering as "packaging-only reverse edge; no code may import against this flow" — which correctly captures the subtlety that the [slurm] extra points "backward" without violating import direction.

Structural Impact

(graphify, 3.5s)

Risk: LOW (localized change)

  • 5 Python files, 0 AST entities, 0/78 clusters

  • Note: changes touch unknown package(s) (data-designer-slurm); analysis may be incomplete - update _PACKAGE_SUBDIRS in structural_impact.py.

Verdict

Needs changes — The publish-pipeline gap (publish.sh) is the one item worth resolving (or explicitly deferring in the PR description) before this ships, since the advertised data-designer[slurm] extra would otherwise break at install time once released. The two suggestions are optional polish.


This review was generated by an AI assistant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add the optional Slurm package and its install extra

1 participant