Skip to content

feat: add Slurm image registry foundation - #891

Open
nabinchha wants to merge 7 commits into
feat/slurm-executionfrom
codex/867-image-registry-foundation
Open

feat: add Slurm image registry foundation#891
nabinchha wants to merge 7 commits into
feat/slurm-executionfrom
codex/867-image-registry-foundation

Conversation

@nabinchha

Copy link
Copy Markdown
Contributor

📋 Summary

Adds the first independently reviewable slice of #867: package-owned digest-bound client and serving inspection, immutable registry records, atomic workspace-derived YAML persistence, and verified alias/path resolution. OCI import, artifact publication, and compatibility hardening remain in the later scoped slices.

🔗 Related Issue

Part of #867

🔄 Changes

  • Add injectable client and vLLM serving inspectors with normalized package errors.
  • Add immutable registered-image and registry-snapshot records.
  • Add locked, atomic, restrictive registry.yaml persistence under the selected workspace.
  • Register existing absolute SQSH files in place and resolve aliases or registered paths with digest/kind re-verification.
  • Add deterministic inspection fakes, concurrency and filesystem-boundary coverage, and isolated wheel checks.
  • Declare and lock the direct PyYAML runtime dependency.

🧪 Testing

  • All package test suites pass (4,514 passed, 1 skipped)
  • Slurm unit tests added/updated (434 passed)
  • Repository-wide Ruff lint and format checks pass
  • Isolated Slurm wheel installation and import-budget check passes
  • E2E tests not applicable to this foundation slice

✅ Checklist

  • Follows commit message conventions
  • Commits are signed off (DCO)
  • Architecture docs not applicable to this internal foundation slice

Add digest-bound client and serving inspection, atomic workspace-derived registry persistence, and verified alias/path resolution.\n\nPart of #867

Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Exercise the production environment reader and verify restrictive atomic registry persistence discovered during review.\n\nPart of #867

Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Persist the package-owned image registry as deterministic safe YAML at the migration plan's canonical path and declare the runtime YAML dependency explicitly.\n\nPart of #867.

Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Reject symlinked registry and lock files, use explicit UTF-8 persistence, and normalize malformed byte content into the package image error hierarchy.\n\nPart of #867.

Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Exercise the public image service from an isolated installed wheel and assert that the wheel metadata carries the runtime PyYAML dependency.\n\nPart of #867.

Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Treat only a genuinely missing registry as empty and reject non-regular registry or lock nodes before reading, locking, or changing permissions.\n\nPart of #867.

Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
@greptile-apps

greptile-apps Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Adds the foundational Slurm image registry, including digest-bound inspection, immutable records, atomic workspace persistence, and verified alias or path resolution.

  • Introduces client and vLLM serving inspectors with normalized image errors.
  • Persists deterministic registry snapshots using file locks and atomic replacement.
  • Registers existing SQSH artifacts and verifies their digest and image kind during registration and resolution.
  • Adds PyYAML as a direct runtime dependency and expands package, concurrency, and filesystem-boundary tests.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
packages/data-designer-slurm/src/data_designer/slurm/images/service.py Implements digest-bound registration and resolution, including final verification that addresses the previously reported stale-facts race.
packages/data-designer-slurm/src/data_designer/slurm/images/registry.py Adds serialized, atomic registry mutation with rollback while both alias and global mutation locks remain held.
packages/data-designer-slurm/src/data_designer/slurm/images/inspection.py Adds injectable client and serving environment inspectors that produce normalized inspection records.
packages/data-designer-slurm/src/data_designer/slurm/images/records.py Defines immutable registry records and validates alias ordering, uniqueness, and consistent path-bound facts.
packages/data-designer-slurm/tests/images/test_registry.py Covers registration, replacement rollback, resolution verification, concurrency, and filesystem boundaries.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Service as SlurmImageService
    participant Registry as ImageRegistry
    participant Disk as Registry/SQSH filesystem
    Caller->>Service: register_existing(request, inspection)
    Service->>Disk: Hash SQSH
    Service->>Service: Verify digest and image kind
    Service->>Registry: register(image)
    Registry->>Registry: Acquire alias and registry locks
    Registry->>Disk: Atomically persist updated snapshot
    Registry->>Disk: Re-hash persisted image path
    alt Verification succeeds
        Registry-->>Service: RegisteredImage
    else Verification fails
        Registry->>Disk: Restore prior snapshot
        Registry-->>Service: ImageVerificationError
    end
Loading

Reviews (2): Last reviewed commit: "fix: rollback stale image registrations" | Re-trigger Greptile

path = Path(request.source)
sqsh_sha256 = compute_file_sha256(path)
_validate_inspection(inspection, expected_kind=ImageKind(request.kind), expected_sha256=sqsh_sha256)
image = RegisteredImage(

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 Registration Persists Stale Image Facts

If another process replaces or modifies the SQSH after compute_file_sha256 returns, registration persists the old digest and inspection without rechecking the file, causing the newly registered image to fail resolution immediately.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/data-designer-slurm/src/data_designer/slurm/images/service.py
Line: 47

Comment:
**Registration Persists Stale Image Facts**

If another process replaces or modifies the SQSH after `compute_file_sha256` returns, registration persists the old digest and inspection without rechecking the file, causing the newly registered image to fail resolution immediately.

---

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

Reverify image bytes while registry mutation locks remain held and restore the previous snapshot if final verification fails.

Part of #867

Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
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.

1 participant