Simplify container spec: single string instead of build dict#60
Conversation
Align with updated ASTRA schema where the container field is a single string — either an image name (pulled) or a path to a Containerfile (built from source). The runtime detects which case via file existence. Removes all dict-based container spec handling (ContainerBuildSpec, build/context/args keys). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The success_criteria field no longer exists in the ASTRA spec. Removes all references from guides, skills (prism-new, prism-build, prism-verify), loop prompt, and eval task YAML files. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The ASTRA package was renamed from 'astra' to 'astra-tools' and published to PyPI. The old git+https dependency fails because pip detects the metadata name mismatch. Switch to the PyPI package. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@claude review this PR, pay particular attention to whether all edits have been correctly propagated |
✅ Eval Results
Graders✅ spec_valid (1.00) Full output |
|
Claude finished @EiffL's task in 4m 17s —— View job PR Review: Simplify container specTasks:
Overall AssessmentThe propagation is consistent and complete across all 15 changed files. The simplification is clean — every place that previously branched on A few minor issues worth addressing: Issues1.
|
| File | Status |
|---|---|
src/prism/container.py |
✅ All dict branches removed; _is_containerfile used consistently |
src/prism/cli.py |
✅ build and status commands updated; boilerplate template correct |
src/prism/dagster/assets.py |
✅ All isinstance(raw, dict) / isinstance(raw, str) guards removed |
tests/test_container.py |
✅ All dict specs replaced with string specs; new _is_containerfile behavior covered |
tests/test_assets.py |
✅ Updated; local_runtime="docker" correctly added |
claude/prism/guides/astra-reference.md |
✅ success_criteria and dict container example removed |
claude/prism/guides/prism-reference.md |
✅ Container description updated |
claude/prism/skills/prism-build/SKILL.md |
✅ success_criteria reference removed |
claude/prism/skills/prism-build/assets/loop-prompt.md |
✅ success_criteria step removed |
claude/prism/skills/prism-new/SKILL.md |
✅ success_criteria and analysis context updated |
claude/prism/skills/prism-verify/SKILL.md |
✅ Success criteria section removed; report template updated |
evals/tasks/snae/CLAUDE.md |
✅ /prism-verify description updated |
evals/tasks/snae/astra.yaml |
✅ Dict spec and success_criteria removed |
CLAUDE.md |
✅ Key invariant updated |
pyproject.toml |
✅ astra-tools>=0.2.2 dependency updated |
One file not changed that you may want to verify manually: claude/prism/skills/prism-migrate/SKILL.md — it mentions "Any existing container setup (Dockerfile, Containerfile)" which is fine and doesn't need updating, but worth a quick read to confirm there's no dict-format example buried in the migration guidance.
Summary
All edits are correctly propagated. The PR achieves its stated goal cleanly. The issues above are minor (dead code, naming convention, unreachable param) and none of them are blockers. The most important one to fix before merge is probably #2 (cross-module private import) since it establishes a convention the codebase has to live with.
|
@claude Can you fix all the issues identified in this review? |
|
Claude finished @EiffL's task in 3m 15s —— View job Fixed all review issuesTasks:
All changes are in |
…ntainerfile, document build_args - Remove `extra: dict[str, Any]` field from ContainerStatus — it was never set or read after the dict-spec removal; also drops the now-unused `field` and `Any` imports - Rename `_is_containerfile` → `is_containerfile` (remove underscore) since cli.py imports it cross-module; update all call sites in container.py and cli.py - Add docstring note to `build_image` and `build_image_podman_hpc` that `build_args` is a low-level-only parameter not reachable via the high-level resolve API - Add `.. warning::` note to `resolve_container_spec` about the silent typo-treated-as-image-name behaviour and how to catch it early Co-authored-by: Francois Lanusse <EiffL@users.noreply.github.com>
Summary
containeris a single string — either an image name (e.g.,python:3.9,ghcr.io/org/img:latest) that gets pulled, or a path to a Containerfile (e.g.,Containerfile,containers/Dockerfile) that gets built from source{"build": "Containerfile", "context": ".", "args": {...}}) — the runtime now detects whether to pull or build by checking if the string points to an existing fileresolve_container_spec,resolve_container_for_slurm,get_container_status, CLIbuildcommand, asset factory, guides, templates, and testsTest plan
prism initcreates correctcontainer: Containerfilein astra.yaml templateprism buildcorrectly detects Containerfile paths vs image namesprism statusshows correct container status for both cases🤖 Generated with Claude Code