feat(profiles): auto-inject variant jupyterlab image for image-variant: gpu profiles - #237
feat(profiles): auto-inject variant jupyterlab image for image-variant: gpu profiles#237tylerpotts wants to merge 4 commits into
image-variant: gpu profiles#237Conversation
Profiles marked gpu: true in jupyterhub.custom.profiles get kubespawner_override.image set to the chart-derived GPU image (<singleuser.image.name>-gpu:<singleuser.image.tag>) unless an explicit image is present. Both jupyterlab images are built from the same commit with the same sha tag, so GPU profiles now track pack updates automatically instead of pinning a -gpu SHA in the deployer overlay. Deployers can override the injected ref chart-wide via jupyterhub.custom.gpu-image. The gpu key is stripped before profiles reach KubeSpawner. Closes #230
|
Docs preview for |
viniciusdc
left a comment
There was a problem hiding this comment.
Nice work — this is the right fix for #230, and the derivation holds up. I checked scripts/bump_image_tags.py:27-30 and TAG_TARGETS does bump singleuser.image.tag, so the derived ref really does track releases with no script change. The suite reproduces too: 137 passed in a CI-equivalent venv, no skips.
One blocker, two questions, and a few smaller things.
Blocker
profile_options.image.choices silently defeats the injection — config/jupyterhub/01-spawner.py:445
A gpu: true profile that also declares an image profile option won't get the GPU image. KubeSpawner applies the choice override after the profile-level one and it replaces rather than merges (_load_profile at spawner.py:3642, profile override at :3658, choices at :3694, and the docstring at :1807-1814 spells out the precedence). Reproduced against the pinned 7.0.0 — profile-level gpu-lab:sha-1 in, cpu-lab:sha-1 out.
The spec says deployer-defined profiles rarely carry them, but both shipped profiles do (values.yaml:523-531 and :541-549), and that's the shape someone copies to build a GPU profile. So you end up with a GPU node requesting nvidia.com/gpu: 1 and running the CPU image, silently — and jhub-apps' Create App reads type.kubespawner_override.image, so it displays the GPU one. Split brain.
Injecting into the choices too would fix it properly, but a log.warning when a gpu: true profile declares profile_options.image, plus a doc sentence, is fine by me.
Questions
Should this be image-variant: gpu rather than a boolean? — values.yaml:495
We already have two naming axes (-base, -gpu), so a -rocm or an ARM build isn't hypothetical. Once gpu: true ships in values.yaml we can't drop it without a deprecation cycle, and we'd end up carrying gpu: true and variant: rocm. The string key is the same amount of code today.
The counter-argument is that gpu: true reads better in an overlay and is more discoverable, which I do buy. What made you pick the boolean? Asking mainly because the spec has no ### Rejected alternatives or ### Why this approach section — the umask spec has both, and this is exactly the call that belongs there.
"always valid" isn't quite true — docs/src/content/docs/server-profiles.md:173-176
It holds for the chart default, but anyone mirroring jupyterhub.singleuser.image.name into ECR or an airgapped registry gets <mirror>-gpu:<tag>, which almost certainly doesn't exist. helm upgrade and helm template both succeed, and the first GPU spawn dies in ImagePullBackOff — no signal until a user hits it.
custom.gpu-image is exactly the escape hatch for this, so that knob earns its place IMO. But neither this page nor values.yaml:393-398 says "if you mirror singleuser.image, set gpu-image too". Also worth a line that jupyterlab-gpu is amd64-only (build-images.yaml:67) while the CPU image is multi-arch.
Suggestions
gpu: false isn't stripped — config/jupyterhub/01-spawner.py:460-461
The strip sits inside if profile.get("gpu"), so gpu: false stays on the profile — but the docstring at :454-456, server-profiles.md:180, and values.yaml:502 all say it's stripped either way. Not a spawn bug, I checked: _load_profile, _get_initialized_profile_list and get_options_form are all happy with gpu: False present, no warnings. So it's just the three docs being wrong.
Hoisting the strip out of the if makes them true and removes the case entirely. I tried it (if "gpu" in profile: outer, if is_gpu: inner) and the suite still passes 137.
The new helper orphans the nebiImage doc comment — templates/_helpers.tpl:161-178
The new comment and define got wedged between the "Nebi image reference" comment at :161-164 and the nebiImage define at :178 it documents, so nebiImage has no doc now and the GPU helper looks like it carries a two-paragraph one. Moving the new block below nebiImage's {{- end -}} fixes it. While you're there — there's a real precedence chain here (explicit kubespawner_override.image > custom.gpu-image > derived > nothing), and every other helper with one carries the numbered "Order of precedence" list (:12, :29, :67, :94).
The injection is invisible in hub logs — config/jupyterhub/01-spawner.py:445
We log nss-wrapper detail at debug (:1007, :1015, :1067) and profile visibility at info (:437), so a derived image ref that gets pulled into a user pod is above that bar. The log.info("profiles: loaded %d profile(s)...") at :475 only prints slugs, so kubectl logs deploy/hub can't answer which image the GPU profile got or where it came from. Adding the injected ref to that line would cover it.
Same place: a log.warning when gpu-image derives empty would help. That case silently lands a CPU image on a GPU node, and raising would break hub startup and therefore login, so a warning is the right level.
Test docstrings, and one doc clause — tests/unit/test_spawner_profiles.py
Four of the seven new tests have no docstring (:286, :295, :309, :330), and none of the seven carries an assert message. test_chart_derived.py:153 is the model — it has both.
Separately, worth one clause in the docs: _bump_profile_list matches startswith(JUPYTERLAB_IMAGE + ":") (:79, :93), so an explicitly pinned -gpu ref never gets bumped. "An explicit kubespawner_override.image always wins" also means "and stays frozen forever", which is the exact pain #230 is about. Worth saying so nobody pins as a workaround.
Follow-ups, not this PR
- Teach
bump_image_tags.pyto bump-jupyterlab-gpu:refs so an explicit pin isn't a one-way door. - Write down the build invariant.
build-images.yamlrunsjupyterlabandjupyterlab-gpuas independent workflow calls with noneeds:, and the only thing keeping the derived ref real isbump-image-tags.yaml:31gating onworkflow_run.conclusion == 'success'. Acontinue-on-error, apaths:divergence, or the manualworkflow_dispatchpath breaks it silently and nothing records that. - Name the
gpustrip next to_PROFILE_GATING_KEYS(:334) with a line on why load-time stripping is correct here, so a future custom key knows which mechanism to use.
Checked and explicitly not raising
The hub does roll on a values change (both paths change a z2jh checksum annotation); tag: 1.2 is rejected by z2jh's schema before printf sees it; the bump_image_tags.py prefix mismatch is deliberate — the trailing colon is what stops it rewriting a -gpu ref into the CPU one; no test isolation leakage from the z2jh.get_config swap.
One worth flagging because it's easy to get backwards: the if and $img.name $img.tag guard in the helper is not dead code. z2jh's schema requires name/tag to be present strings but allows empty ones, so --set jupyterhub.singleuser.image.name="" is schema-valid and renders. With the guard you get "gpu-image": ""; without it, "-gpu:sha-5dfee5e", a garbage ref. The same reasoning makes the Python if gpu_image and ... guard reachable. Keep both, and keep test_gpu_profile_without_derived_image_falls_back_to_default.
Nothing security-relevant here either — gpu and custom.gpu-image are both deployer-authored values, and the deployer can already write kubespawner_override.image directly, which is what #230 is asking to stop doing. The new knob is strictly less expressive than what they already control.
- warn when a gpu: true profile declares profile_options.image, since the selected choice's image replaces the injected one at spawn time - strip the gpu key whatever its value (gpu: false was leaking through) - warn when no GPU image can be derived and custom.gpu-image is unset - log the injected ref per profile and the chart-wide gpu-image at load - document the gpu strip mechanism next to _PROFILE_GATING_KEYS - _helpers.tpl: move gpuJupyterlabImage below nebiImage so nebiImage keeps its doc comment; add the numbered precedence list - docs/values: drop 'always valid'; mirrored-registry + amd64-only notes; explicit pins are never bumped by bump_image_tags.py; profile_options caveat - spec: add Why this approach / Rejected alternatives - tests: docstrings + assert messages, cover the new warnings and logs
Per review: a string key generalises to future -rocm/arm64 variants without carrying a boolean alongside it. The derivation moves from Helm into 01-spawner.py (<singleuser.image.name>-<variant>:<tag>) because only the profile knows the variant name; custom.gpu-image becomes the custom.image-variants map of per-variant full-ref overrides. Drops the gpuJupyterlabImage helper and the _CHART_DERIVED entry.
gpu: true profilesimage-variant: gpu profiles
|
Thanks for the thorough review — addressed in da477a7 and c6f2201. Blocker:
"always valid". Reworded to "exists on quay.io/nebari for every release", and added a Suggestions
Follow-ups (left out as you suggested): teaching PR title/body updated to match. Suite: 141 passed; |
|
This has been tested on Hetzner and is functional |
What
Closes #230
Profiles in
jupyterhub.custom.profilescan now be markedimage-variant: <name>:At hub startup,
01-spawner.pyinjectskubespawner_override.image = <singleuser.image.name>-<variant>:<singleuser.image.tag>, so GPU profiles track pack updates exactly like CPU profiles instead of pinning a-gpuSHA in the deployer overlay.gpuis the only variant published today; the key is a string so a future-rocm/arm64 build needs no chart change.How
build-images.yamlrun from the same commit, so they always share the samesha-<short>tag._resolve_image_variants()in01-spawner.pyruns once at load, readingsingleuser.image.name/.tagandcustom.image-variantsfrom z2jh. Precedence: explicitkubespawner_override.image>custom.image-variants.<name>> derived > nothing (warn, fall back to CPU default). Theimage-variantkey is stripped whatever its value.jupyterhub.custom.image-variants: {}is the per-variant full-ref override map — the escape hatch for mirrored/airgapped registries.profile_options.image(the selected choice's image replaces the injected one at spawn time) and logs the injected ref per profile.scripts/bump_image_tags.pyalready bumpssingleuser.image.tageach release, so the derived ref stays current with zero script changes.Testing
tests/unit/test_spawner_profiles.py: derivation, generic variant names, override map, explicit-image precedence, key stripping (incl. empty variant), empty-base fallback + warning,profile_options.imagewarning (+ negative), input non-mutation, load-time wiring from z2jh keys, load-time log naming the injected ref (13 tests).helm lint/helm template(incl.--set nebariapp.enabled=falseas CI does) andruff check config/clean.Docs
docs/src/content/docs/server-profiles.md: "The GPU image, without hardcoding a SHA" section, mirrored-registry caution, amd64-only note,profile_options.imageand explicit-pin caveats.docs/src/content/docs/values-reference.md:image-variantsrow.values.yaml:image-variantsplaceholder + updated GPU profile example.docs/superpowers/specs/2026-08-24-gpu-profile-image-design.md: design, why the string key, rejected alternatives.