Skip to content

fix(lib): give local dry-run services an id so rendering works - #528

Open
cswaney wants to merge 2 commits into
mainfrom
fix/494-dry-run-local-profile
Open

fix(lib): give local dry-run services an id so rendering works#528
cswaney wants to merge 2 commits into
mainfrom
fix/494-dry-run-local-profile

Conversation

@cswaney

@cswaney cswaney commented Aug 28, 2026

Copy link
Copy Markdown
Member

Summary

  • blackfish run --dry-run crashed with AttributeError: 'NoneType' object has no attribute 'hex' for local profiles. The local branch constructed its service without an id, unlike the Slurm branch, and render_job_script reads self.id.hex. A dry-run service is never flushed to the database, so nothing else assigns one. Fixed by adding id=uuid4() to the local constructors in both text_generation.py and speech_recognition.py.
  • The four dry-run tests patched the service class, so the real renderer never ran and the crash was invisible. They now use the real service and assert on the script body, not just the metadata echoed above it.
  • That change exposed a second gap: mock_config left CONTAINER_PROVIDER as a bare MagicMock, so the local templates — which branch on provider == "docker" — rendered an empty script. The fixture now sets ContainerProvider.Docker.

Test plan

  • Reproduced the crash on main via CliRunner (exit 1, AttributeError); the same invocation now exits 0 and prints a full docker run script.
  • Confirmed the updated tests fail without the source fix and pass with it — the point of the change.
  • cd lib && uv run pytest tests/cli → 142 passed.
  • cd lib && uv run just lint → all hooks pass (ruff, ruff format, mypy, codespell).
  • Coverage badge regenerated: 71% → 72%.

Closes #530


Generated by Claude Code

claude added 2 commits August 28, 2026 13:29
`blackfish run --dry-run` crashed with `AttributeError: 'NoneType' object
has no attribute 'hex'` for local profiles: the local branch built its
service without an `id`, unlike the Slurm branch, and `render_job_script`
reads `self.id.hex`. Since a dry-run service is never flushed to the
database, nothing else assigns one.

Add `id=uuid4()` to the local constructors in both `text_generation` and
`speech_recognition`, matching the Slurm branch.

The dry-run tests missed this because they patched the service class, so
the real renderer never ran. Drop those patches and assert on the script
body rather than only the metadata echoed above it. `mock_config` now
carries a real `CONTAINER_PROVIDER`, since the local templates branch on
`provider == "docker"` and would otherwise render an empty script.

Closes #494
The dry-run tests now exercise the real job-script renderer and templates
instead of a mocked service, which covers a few more lines.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011Aj5BVf85qjSaKT9kJxap8
@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown

Review

Solid, well-scoped bug fix. I traced the root cause and confirmed it in lib/src/blackfish/server/services/base.py:710: render_job_script reads uuid=self.id.hex, and Service.id (a UUIDAuditBase mapped column) is only populated on DB flush — which never happens for a --dry-run invocation. The Slurm branch already passed id=uuid4() explicitly (line 149/146 in text_generation.py/speech_recognition.py), so the local branch was simply missing the same line. The fix is minimal, correct, and consistent across both service files.

Code quality

  • uuid4 is already imported locally inside the function (from uuid import uuid4), so no new import needed — good catch reusing it rather than adding a redundant top-level import.
  • The fix mirrors the existing Slurm branch pattern exactly, which is the right call for consistency rather than introducing a different approach.

Test coverage — this is the best part of the PR

  • Removing the patch(...TextGeneration...)/patch(...SpeechRecognition...) mocks was the key insight: the previous tests mocked out the exact code path that was broken, so they could never have caught this regression. Asserting on the real rendered script body ("docker run" / "#SBATCH") closes that gap.
  • Catching the second bug (mock_config.CONTAINER_PROVIDER being an unconfigured MagicMock, causing the docker template to silently render empty) via the same change is a nice bit of due diligence — it would have made the new assertions fail for the wrong reason if left unfixed.
  • Minor nit (non-blocking): script = result.output.split("> image_ref:")[-1] is a slightly fragile way to isolate the script body — it'll silently produce an empty/wrong slice if the echoed field list above it is ever reordered or renamed. Not worth blocking on, but a comment noting this is coupled to the click.echo sequence above it (as you already did) is good; alternatively splitting on the "👇 Here's the job script 👇" marker would be a bit more robust to field-list changes, at the cost of matching an emoji string instead.

Other

  • The mock_config.HOME_DIR fixture still returns a 1-tuple ((Path(...),)) rather than a Path — this is pre-existing and out of scope for this PR, just flagging in case it's not intentional.
  • No security or performance concerns — this is a test-only/dry-run code path with no external inputs beyond what's already validated upstream (model/profile lookups).

Nice, tight fix with a good "test the real path, not the mock" instinct. LGTM.

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.

blackfish run --dry-run crashes for local profiles

2 participants