Skip to content

Add Podman support, venv fallback backend, and fix local target crash#41

Merged
EiffL merged 2 commits into
mainfrom
fix/podman-venv-backend-issue-40
Mar 20, 2026
Merged

Add Podman support, venv fallback backend, and fix local target crash#41
EiffL merged 2 commits into
mainfrom
fix/podman-venv-backend-issue-40

Conversation

@EiffL
Copy link
Copy Markdown
Member

@EiffL EiffL commented Mar 19, 2026

Summary

Fixes #40prism run with target: local crashes when Docker is unavailable.

  • Bug fix: build_definitions() no longer unconditionally calls docker build — it detects the available container runtime first and skips building when none is found
  • Podman support: All container operations (build, run, inspect) now accept a runtime parameter, supporting both Docker and Podman as drop-in alternatives for local execution
  • Venv fallback backend: When no container runtime is available, recipes execute in the project's .venv/ (created by prism init) with dependencies auto-installed from requirements*.txt using a hash-based marker to skip redundant installs
  • Container failure fallback improved: When a container run fails, the fallback now goes to _run_venv() (with deps installed) instead of bare _run_local() (without deps)
  • Init guidance: prism init detects and displays the container runtime, or shows installation guidance for Docker/Podman
  • prism build --runtime podman: Added podman as a runtime choice and auto-detection when no runtime is specified

Test plan

  • All 238 tests pass (15 new tests added)
  • ruff check clean on changed files
  • mypy — no new errors (pre-existing only)
  • Manual: prism run with Docker available — unchanged behavior
  • Manual: prism run with only Podman — builds and runs with Podman
  • Manual: prism run with neither — installs deps into .venv and runs there
  • Manual: prism init without container runtime shows guidance message

🤖 Generated with Claude Code

…#40)

When target is "local" and Docker is unavailable, `prism run` crashed
because `build_definitions()` unconditionally attempted `docker build`.
Even with `--no-build`, the local fallback failed because recipe
dependencies weren't installed.

This fixes the crash by detecting the available container runtime
(Docker or Podman) before attempting builds, and adds a venv-based
fallback that installs requirements.txt dependencies into the project's
existing .venv when no container runtime is available.

Changes:
- Add `detect_container_runtime()` to probe for Docker/Podman on PATH
- Parameterize container operations with `runtime` to support Podman
- Add `_run_venv()` backend using the project .venv from `prism init`
- Hash-based dependency marker (.venv/.deps-hash) skips redundant installs
- Container failure now falls back to venv (not bare local) with deps
- `prism init` shows container runtime guidance when none is detected
- `prism build` accepts `--runtime podman` and auto-detects runtime
- Extract shared helpers: `_substitute_python()`, `hash_file_contents()`

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@EiffL
Copy link
Copy Markdown
Member Author

EiffL commented Mar 19, 2026

This PR aims to close #40 , a problem reported by @aboucaud when trying to run dagster locally on a machine that doesn't have docker available

@EiffL
Copy link
Copy Markdown
Member Author

EiffL commented Mar 19, 2026

@claude please review this pr

@claude
Copy link
Copy Markdown

claude Bot commented Mar 19, 2026

Claude finished @EiffL's task in 2m 25s —— View job


PR Review: Add Podman support, venv fallback backend, and fix local target crash

Tasks:

  • Gather context and understand the PR
  • Review src/prism/container.py
  • Review src/prism/dagster/runner.py
  • Review src/prism/dagster/assets.py
  • Review src/prism/cli.py
  • Review test files
  • Post final review

Overall Assessment: This is a solid, well-structured PR that addresses a real user pain point. The architecture is clean — detect → fallback → run — and the hash-based dep marker is a nice touch. A few bugs and rough edges are worth addressing before merge.


Issues Found

🐛 Bug: Failed pip installs are silently swallowed and marker is still written

src/prism/dagster/runner.py:315-331

for req_file in req_files:
    install_result = subprocess.run(...)
    if install_result.returncode != 0:
        logger.warning(...)   # warns but continues

marker.write_text(current_hash + "\n")  # written regardless of failures
self._venv_deps_checked = True

If pip install fails (e.g. a package doesn't exist or there's a network error), the marker file is still written with the current hash. On the next run, the hash matches and installation is skipped entirely — the broken dep state becomes permanent until requirements.txt changes. The marker should only be written when all installs succeed.

Fix this →


🐛 Regression: Container fallback now errors when .venv is absent

src/prism/dagster/runner.py:126-139

The old behavior: docker backend fails → fall back to _run_local → run in current Python environment (always works).

The new behavior: docker backend fails → fall back to _run_venverror if .venv doesn't exist.

For users upgrading from an existing install who haven't run prism init (or used --no-venv), a container failure now yields a confusing "No .venv found" error rather than running. The prism run command on a pre-existing project with no .venv will break silently where it used to work.

A secondary _run_local fallback after _run_venv failure, or at least a more actionable error message pointing to prism init, would improve the UX.


🐛 Formatting bug in prism init guidance message

src/prism/cli.py:285-290

console.print(
    "\n[yellow]Note:[/yellow] No container runtime (Docker or Podman) detected.\n"
    "  Recipes will run in the project venv "
    "(dependencies from requirements.txt).\n"
    "  For full container isolation, install one of:\n"
    "    Podman: [cyan]https://podman.io/docs/installation[/cyan]"
    "  (recommended — rootless, no daemon)\n"      # ← no \n before this line
    "    Docker: [cyan]https://docs.docker.com/engine/install/[/cyan]"
)

The (recommended — rootless, no daemon) annotation is concatenated directly onto the Podman URL string (no \n separator), so it renders on the same line as the URL. Should be:

    "    Podman: [cyan]https://podman.io/docs/installation[/cyan]\n"
    "  (recommended — rootless, no daemon)\n"

Fix this →


Design Observations

Double container resolution in build_definitions + build_asset_definitions

src/prism/dagster/assets.py:237-266

build_definitions calls _resolve_container to compute default_container and passes it to ASTRAContainerRunner. Then build_asset_definitions (called immediately after) also calls _resolve_container independently for the same spec. For a build spec this means the image is built/checked twice. Not a correctness issue since the second call hits the cache, but it's wasteful.


Docker preferred over Podman in detection

src/prism/container.py:42-45

for runtime in ("docker", "podman"):
    if shutil.which(runtime) is not None:
        return runtime

Docker is checked first. On systems with both (e.g., macOS with Docker Desktop and Podman), Docker wins silently. Since Podman is typically rootless and daemon-free (making it more appropriate as a local development runtime), preferring it might be more user-friendly — or at least worth documenting. Consider adding a note in the docstring.


_venv_deps_checked is per-instance but hash check is global

src/prism/dagster/runner.py:299-331

The _venv_deps_checked flag is an in-memory instance cache that prevents the hash-file check on every execute() call. This is fine for Dagster runs (one runner per build_definitions call), but it means that if requirements change during a multi-asset run, they won't be picked up. Since prism run creates one runner per dg.materialize call, this is unlikely to matter in practice — just worth knowing.


Test Coverage Notes

  • TestContainerToVenvFallback tests docker→venv fallback ✓
  • Missing: a test for the case where backend="docker", no container is specified, and no .venv exists — this now returns an error rather than running, which is a behavior change worth asserting explicitly.
  • test_venv_installs_requirements uses six; test_venv_deps_hash_changes_on_new_requirements uses chardet. These are not in the project's test dependencies — might want to use something guaranteed to be available, or add them to [dev] extras.

Minor / Nit

  • Windows: _run_venv hardcodes venv_path / "bin" / "python" (Unix-only). Non-blocking given the HPC/Linux focus, but the pattern in _create_venv (which checks sys.platform == "win32") could be applied here too.
  • The local backend (explicit target) still uses _run_local without deps, while the implicit venv backend installs deps. This asymmetry is intentional but slightly surprising — worth a comment in execute() to clarify.

Summary: The core logic is correct and well-tested. The main items to fix before merge are the pip-marker bug (silent dep install failures become permanent) and the regression where a missing .venv now surfaces as an error for the docker→fallback path.

@EiffL
Copy link
Copy Markdown
Member Author

EiffL commented Mar 19, 2026

@claude can you address the review posted above?

@claude
Copy link
Copy Markdown

claude Bot commented Mar 19, 2026

Claude finished @EiffL's task in 4m 12s —— View job


Addressed PR Review Issues

Tasks:

  • Gather context and read relevant files
  • Fix bug: marker written even when pip install fails (runner.py)
  • Fix regression: container fallback crashes when .venv absent (runner.py)
  • Fix formatting bug in prism init guidance message (cli.py)
  • Add test for docker backend with no .venv present
  • Fix test packages (six, chardet) — replaced with pip/setuptools
  • Add comment clarifying local vs venv backend asymmetry
  • Commit and push all fixes

All issues from the review have been addressed in commit 942d1a0. Here's a summary of each change:

src/prism/dagster/runner.py

Bug: pip failure marker written regardless of success_ensure_venv_deps now tracks whether all installs succeeded with an all_installed flag and only writes .deps-hash when every pip install -r call returned 0. A failed install will no longer be permanently cached.

Regression: missing .venv surfaces as error — The container backend fallback now checks whether .venv/bin/python exists before calling _run_venv. When .venv is absent, it falls back to _run_local with an actionable warning message, restoring the pre-PR behavior for users who haven't run prism init.

Comment: local vs venv asymmetry — Added a docstring comment in execute() explaining that the explicit "local" backend skips dep installation (by design), while the implicit fallback installs deps via _run_venv when .venv is present.

src/prism/cli.py

Formatting bug — Added the missing \n after the Podman URL so (recommended — rootless, no daemon) renders on its own line.

tests/test_runner_venv.py

Test packages — Replaced six and chardet (not in dev deps, require network) with pip and setuptools, which are always pre-installed in any Python venv and can be "installed" without network access.

Missing tests — Added two new tests in TestContainerToVenvFallback:

  • test_docker_no_venv_falls_back_to_local: asserts that when a container run fails and .venv is absent, the result is exit_code=0 with backend="local" (not an error)
  • test_docker_no_container_no_venv_falls_back_to_local: same for the no-container path
    | Branch: fix/podman-venv-backend-issue-40

…atting

- runner.py: Only write .deps-hash marker when all pip installs succeed,
  preventing a failed install from being silently skipped on the next run
- runner.py: When .venv is absent in the docker backend fallback path, fall
  back to _run_local instead of returning a confusing error, restoring
  pre-PR behaviour for projects that predate prism init
- runner.py: Add comment clarifying local vs venv backend asymmetry in execute()
- cli.py: Fix missing newline after Podman URL in prism init guidance message
- test_runner_venv.py: Replace six/chardet with pip/setuptools (always in venv)
- test_runner_venv.py: Add tests for docker backend with no .venv present

Co-authored-by: Francois Lanusse <EiffL@users.noreply.github.com>
@EiffL
Copy link
Copy Markdown
Member Author

EiffL commented Mar 19, 2026

@claude review this PR

@claude
Copy link
Copy Markdown

claude Bot commented Mar 19, 2026

Claude finished @EiffL's task in 1m 31s —— View job


PR Review: Add Podman support, venv fallback backend, and fix local target crash

Tasks:

  • Gather context
  • Read changed files (runner.py, container.py, assets.py, cli.py, test_runner_venv.py, test_container.py)
  • Analyze code quality and correctness
  • Post final review

Overall Assessment: This is a well-executed PR that addresses a real user pain point. All three bugs from the previous review have been correctly fixed. The code is clean and the test coverage is solid. A few very minor nits remain, none blocking.


All Previous Issues Addressed ✅

Bug: pip failure marker written regardless of success → Fixed correctly. runner.py:337-356 now tracks all_installed and only writes .deps-hash when every pip install -r returns 0.

Regression: container fallback crashes when .venv absent → Fixed correctly. runner.py:138-160 checks for .venv/bin/python before calling _run_venv, falling back to _run_local with an actionable warning message.

Formatting bug in prism init guidance message → Fixed. cli.py:287 now has the \n after the Podman URL.

Test packages six/chardet → Replaced with pip/setuptools. New tests for .venv-absent fallback paths added at test_runner_venv.py:153-183.


Remaining Minor Items

Nit: _venv_deps_checked set to True even on partial failure

src/prism/dagster/runner.py:355-357

if all_installed:
    marker.write_text(current_hash + "\n")
self._venv_deps_checked = True  # ← set unconditionally

When a pip install fails, all_installed is False and the marker is correctly not written. However, _venv_deps_checked is still set to True, which means if the same runner instance executes multiple assets in sequence (one dg.materialize call), a dep install failure in the first asset silently skips the install for all subsequent assets in the same run. The marker being absent means the next run will retry, but within the same run, the failure is cached. Since prism run creates one runner per materialization, this is a narrow window but worth knowing about.

Fix this →


Nit: detect_container_runtime prefers Docker silently

src/prism/container.py:42-45

for runtime in ("docker", "podman"):
    if shutil.which(runtime) is not None:
        return runtime

Docker wins on systems with both installed (e.g. macOS with Docker Desktop + Podman). The TestDetectContainerRuntime test at test_container.py:342 asserts this behavior explicitly, so it's intentional — but the docstring at line 33 could note the preference so users who have both can set --runtime podman explicitly to override.


Nit: _ensure_venv_deps doesn't cache the no-requirements-files case

src/prism/dagster/runner.py:326-328

req_files = [f for f in dep_files if f.name.startswith("requirements")]
if not req_files:
    return  # returns without setting _venv_deps_checked

When there are no requirements*.txt files, the function returns early without setting _venv_deps_checked = True. Every subsequent execute() call on the same runner re-scans the filesystem unnecessarily. Trivial, but easy to fix by setting the flag before the early return.


Design Note (unchanged from before)

The double _resolve_container call in assets.py (once in build_definitions at line 239, once in build_asset_definitions at line 72) remains. This is the same non-blocking issue noted previously — the second call hits the image cache, so it's not a correctness problem. Happy to see it addressed in a follow-up.


Summary: The core bugs are all fixed and the tests are solid. The three items above are all minor nits. This PR is ready to merge as-is — the nits can be addressed in a follow-up if desired.
| Branch: fix/podman-venv-backend-issue-40

Copy link
Copy Markdown
Member Author

@EiffL EiffL left a comment

Choose a reason for hiding this comment

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

LGTM

@EiffL EiffL merged commit 448be40 into main Mar 20, 2026
5 of 9 checks passed
@aboucaud aboucaud deleted the fix/podman-venv-backend-issue-40 branch May 8, 2026 10:46
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.

prism run attempts Docker build when target is local

1 participant