Skip to content

feat(web): add image version selectors to both launchers - #490

Merged
cswaney merged 3 commits into
mainfrom
feat/image-version-selectors
Aug 22, 2026
Merged

feat(web): add image version selectors to both launchers#490
cswaney merged 3 commits into
mainfrom
feat/image-version-selectors

Conversation

@cswaney

@cswaney cswaney commented Aug 22, 2026

Copy link
Copy Markdown
Member

Summary

  • Adds a Version select to both launchers, offering only the tags actually staged on the profile (from GET /api/containers, Add GET /api/containers to list staged container images per profile #482) so a selection can always run. The choice is sent as image_ref, which the backend persists and reuses across restarts (Persist and honor a pinned container image (image_ref) #480).
  • Collapsed into Advanced Options in both launchers. The configured default is right for almost everyone, and pinning is a reproducibility need rather than a routine choice — a prominent control would invite changing something that can only make things worse for a casual user. This meant adding an Advanced section to the local-profile branch of the service form (local profiles run containers too, and previously had none) and one to the batch wizard's Model step, the only step both profile types share.
  • Pre-selects the configured default, not the newest staged tag. The pre-selection should change when someone deliberately changes configuration — never because an administrator staged a new image. When that default is missing from disk, it falls back to the first staged tag so the offer is still runnable.
  • The two launchers resolve different images, which is easy to get wrong: services map their hyphenated pipeline name to a config.IMAGES key (text-generationtext_generation), while batch jobs always use tigerflow_ml. TASKS[].service is an HF pipeline tag used to filter which models to offer (image-text-to-text, object-detection) — it is not an image key, and every batch task runs the tigerflow-ml container regardless (jobs/base.py resolves images["tigerflow_ml"] with no task branch).

This is the last of four sub-issues under #212, so image version pinning is now complete end to end: select a version, it is recorded, restarts reuse it, and the CLI and UI show it.

Behavior worth reviewing

Two empty cases, handled oppositely. They look the same in the UI but mean different things, and the route distinguishes them:

Case container Behavior
Profile unreachable / discovery failed undefined Launch proceeds on the backend's configured default — a flaky login node must not block a launch
Discovery succeeded, nothing staged {tags: []} Model step blocked, with an error naming the fix

The second case would otherwise fail at pre-flight with "tigerflow-ml image not found" after the user filled in all four wizard steps. Blocking early is the friendlier failure. Note this deliberately does not gate the step on imageRef being set, which would have conflated the two cases and broken the unreachable path.

A single staged version still renders a select, matching RevisionSelect, rather than hiding below two options. Consistency beats a special rule the user would have to learn; disabling one-option selects uniformly is tracked in #489.

Hyphen conversion uses replaceAll. runService uses a non-global replace, which would turn image-text-to-text into image_text-to-text. That is latent today — only speech_recognition and text_generation are launchable services and both have a single hyphen — but the new code should not inherit it.

Test plan

  • npm run lint — 0 errors (one pre-existing warning in an untouched file). npm test — 522 passed across 53 files (9 new).
  • New ImageVersionSelect.test.jsx covers: pre-selecting the configured default (using a container where the default is deliberately not the newest tag, so the assertion is meaningful), lifting the full repo:tag, falling back when the default is unstaged, rendering nothing when unreachable and when nothing is staged, keeping a single-option select, re-seeding on a container change, and not crashing without a setter.
  • Verified the default-selection tests actually fail when the logic is swapped for "newest staged tag" — two of them catch it.
  • ServiceModal.test.jsx needed both a useStagedContainers mock (the auto-mock returns undefined and crashes on destructure) and an updated runService payload assertion for the new argument.
  • Checked live in the running app.

Closes #483.

Offer only the versions actually staged on the profile, so a selection
can always run, and send the choice as image_ref.

Pre-select the configured default rather than the newest staged tag: the
pre-selection should change when someone changes config, not because an
admin staged a new image. Fall back to the first staged tag when that
default is missing from disk.

The two launchers resolve different images. Services map their
hyphenated pipeline name to a config.IMAGES key; batch jobs always use
tigerflow_ml, since every task runs in that container regardless of
TASKS[].service, which is an HF pipeline tag for filtering models.

Block the batch Model step only when discovery succeeded and reported
nothing staged — the job would fail its pre-flight, so stop there rather
than four steps later. An unreachable profile still launches on the
backend default.

Closes #483.
@claude

claude Bot commented Aug 22, 2026

Copy link
Copy Markdown

Review

Nicely scoped, and the write-up in the PR description does a lot of the reviewer's work (the two-empty-cases table and the replace vs replaceAll callout are exactly the things I'd have flagged). Backend contracts (/api/containersStagedContainer, image_ref on ServiceRequest/job model) line up with what the frontend sends and expects, so this looks safe to merge as frontend-only. A couple of things worth a look:

ImageVersionSelect.jsx — the seed/lift effects can briefly fire out of sync

```js
useEffect(() => { // seeds `selected` from `container`
...
setSelected(container.default_staged ? container.default : fallback);
}, [container]);

useEffect(() => { // lifts `${container.repo}:${selected}`
...
setImageRef(`${container.repo}:${selected}`);
}, [selected, container, setImageRef]);
```

Both effects depend on `container`. When `container` changes (e.g. switching between a vLLM service and a tigerflow job), they run in declaration order in the same commit: the lift effect fires first with the new `container` but the stale `selected` from the previous container, calling `setImageRef` with a mismatched repo:tag pair for one tick before the seed effect's `setSelected` triggers a re-render that corrects it. The "re-seeds when container changes" test only asserts `toHaveBeenLastCalledWith`, so this transient bad call is invisible to the suite.

In practice this self-heals before a user could act on it (no paint/interaction happens in between), so I wouldn't block on it, but it's a bit fragile — relying on effect ordering to mask a wrong intermediate value is easy to break with a future refactor. Simplest fix would be to derive `selected` and the lifted ref from `container` in one effect (or a `useMemo`) instead of chaining two effects through state.

`NewJobModal.jsx` — no test coverage for the new step-blocking logic

`noContainerStaged` and the `isStepValid` `"model"` case gating are new decision logic with real consequences (blocks the wizard, shows an error alert), but `NewJobModal.test.jsx` only exercises the pure helper exports (`coerceParamValue`, `getDefaultTaskParams`, etc.) — there's no render-level test covering the "discovery succeeded but nothing staged" vs. "profile unreachable" distinction for the batch launcher, even though `ImageVersionSelect.test.jsx` and the updated `ServiceModal.test.jsx` cover the equivalent cases for the service launcher. Given how deliberately the PR description calls out this behavior as worth reviewing, a regression test would be good here — could be as small as extracting the `noContainerStaged` condition into an exported pure function, matching the pattern already used for the other helpers in this file.

Nit

`runService`'s existing `pipeline.replace("-", "_")` (non-global, in `requests.js`) is called out in the PR description as latent and intentionally untouched — agreed it's out of scope, but since the new `image_ref` line sits immediately next to it and the exact bug is already root-caused, it might be worth a one-line fix now rather than a follow-up, to avoid the footgun surviving into the next service with two hyphens in its name.

Nice touches

  • The `default` vs `default_staged` fallback logic and its dedicated test (deliberately choosing a non-newest default) is a good, meaningful regression test.
  • Distinguishing "discovery failed" (`undefined`, launch proceeds) from "discovery succeeded, nothing staged" (`{tags: []}`, block early) is a genuinely good UX call, correctly implemented in both `ImageVersionSelect` and `NewJobModal`.
  • `encodeURIComponent` on the profile name in `fetchStagedContainers` — good hygiene, no injection concerns.
  • SWR key `containers/${profile.name}` (service excluded) correctly dedupes a single fetch across both launchers for the same profile, since the endpoint returns all services and filtering happens client-side.

Overall: solid, well-tested change. The effect-ordering issue is the only thing I'd actually want addressed or consciously accepted before merge; the test-coverage gap is a nice-to-have.

Seeding `selected` in one effect and lifting it in another let the lift
fire first on a container change — with the new repo but the previous
tag, emitting a reference that does not exist. Derive the selection
instead, so the two can never disagree, and keep only the user's
explicit pick in state.

Collapse the selector into Advanced Options in both launchers. The
configured default is right for almost everyone; pinning is a
reproducibility need, so it should not invite casual changes. Local
service profiles gain an Advanced section, since they run containers
too, and the batch wizard gets one on its Model step, the only step both
profile types share.

Extract isNoContainerStaged so the step-blocking rule is testable, and
use replaceAll for the pipeline-to-image-key conversion.
@cswaney

cswaney commented Aug 22, 2026

Copy link
Copy Markdown
Member Author

Triage of the automated review — all three addressed (ba29e7b).

Effect ordering — confirmed real, and I could reproduce it. Instrumented the container switch and captured every setImageRef call:

[ "ghcr.io/princeton-ddss/tigerflow-ml:v0.20.0" ]   <- new repo, previous tag
[ "ghcr.io/princeton-ddss/tigerflow-ml:0.1.1"  ]

That first reference does not exist, and it would be the pinned value if a submit landed in that window. Fixed by deriving the selection from container rather than chaining two effects through state; only the user's explicit pick is now held in state, and it is dropped when the container changes. After the fix the same instrumentation shows a single correct call.

Added a regression test that asserts every call is correct, not just the last — toHaveBeenLastCalledWith is exactly what let this hide.

Step-blocking coverage. Extracted isNoContainerStaged as an exported pure function, matching the existing helper pattern in that file, with four tests covering the distinction the PR description calls out: blocks when discovery found nothing staged; does not block when a version is staged, when the profile is unreachable, or while the fetch is still in flight.

Non-global replace. Agreed it was worth fixing in place rather than deferring — the image_ref line sits directly beneath it and the bug was already root-caused. Now replaceAll, with a comment on why it matters (image must match a config.IMAGES key exactly).


Separately, on review feedback the selector has moved into Advanced Options in both launchers. The configured default is correct for almost everyone, and pinning is a reproducibility need rather than a routine choice — a prominent control invited changing something that can only make things worse for a casual user. This also meant adding an Advanced section to the local-profile branch of the service form (local profiles run containers too) and one to the batch wizard's Model step, the only step both profile types share. The PR description has been updated.

527 tests passing; 0 lint errors.

Expanding Advanced or Deployment Options below the fold revealed nothing
until the user scrolled, so the click looked like it had done nothing.
The batch launcher already scrolled; the service launcher never did.

Share one useScrollOnExpand hook rather than repeating the effect in
each of the four disclosures. The modal owns the scroll container and
passes it down by context, since the collapsible sections live in
nested forms.

Also covers the batch Model step's new section, and clears the pending
timeout on collapse, which the original batch effect did not.
@cswaney
cswaney merged commit 855ef05 into main Aug 22, 2026
3 checks passed
@cswaney
cswaney deleted the feat/image-version-selectors branch August 22, 2026 11:00
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 image version selectors to the service and batch job launchers

1 participant