Repository navigation
feat(notebook): add a notebook plugin - #300
Fan Shangxiang (MartinForReal) wants to merge 6 commits into
Conversation
Fan Shangxiang (MartinForReal)
changed the title
68fd6bd to
7bf8943
Compare
8ccece0 to
45fb5b1
Compare
|
Pengfei Ni (@feiskyer) Kevin Cho (@chokevin) Could you please take a look? Thanks! |
45fb5b1 to
faf4b7f
Compare
| if not path.is_file(): | ||
| raise FileSelectionError(f"{name!r} was not found beside the notebook") | ||
| try: | ||
| data = path.read_bytes() |
There was a problem hiding this comment.
Read at most MAX_FILE_BYTES + 1 bytes before checking the limit. The preview endpoint accepts filenames independently of the picker, and this read allocates the entire file before rejecting it. Selecting a multi-GB companion file, or a file that grew after selection, can exhaust the Jupyter server's memory instead of returning 413. A stat check alone would still race with file growth; add coverage that verifies the read itself is bounded.
There was a problem hiding this comment.
Fixed in 61c2f03: the preview read is bounded at the open/read call rather than after the fact, with
a test that wraps Path.open and asserts the exact byte count, so the old read_bytes() behavior
fails it.
| const sessions = await (await fetch(`${API}/api/sessions?token=${TOKEN}`)).json(); | ||
| for (const session of sessions) { | ||
| await fetch(`${API}/api/sessions/${session.id}?token=${TOKEN}`, { method: "DELETE" }); | ||
| console.log(`stale session ${session.id}: deleted`); | ||
| } | ||
| const kernels = await (await fetch(`${API}/api/kernels?token=${TOKEN}`)).json(); | ||
| for (const kernel of kernels) { | ||
| await fetch(`${API}/api/kernels/${kernel.id}?token=${TOKEN}`, { method: "DELETE" }); |
There was a problem hiding this comment.
Limit cleanup to resources created by this test, or require an isolated disposable Jupyter server. The configured server can already contain other notebooks, but these loops delete every session and kernel without checking ownership or whether they are stale, terminating unrelated computations and losing in-memory results. The same issue exists in run-plugin-e2e.mjs and run-embed-e2e.mjs; button/embed also clear the shared default workspace.
There was a problem hiding this comment.
Addressed originally, and tightened in 4ad40e0 per the follow-up: the harness no longer scans the
server's sessions or kernels at all, so there is no filtering rule left that could match unrelated
work.
| // and lose in-memory results. Matching on the harness's own session name and | ||
| // notebook path keeps cleanup scoped to our runs. | ||
| const sessions = await (await fetch(`${API}/api/sessions?token=${TOKEN}`)).json(); | ||
| const owned = sessions.filter((s) => s.name === SESSION_NAME || s.path === NOTEBOOK); |
There was a problem hiding this comment.
An existing user session for this example matches the notebook path even when its name is unrelated to the harness. The following DELETE requests terminate that session and its kernel, losing in-memory work; fixed session names can also match another active test invocation. Clean up only exact session/kernel IDs created by the harness, use unique run identities, and remove the same path-only match from the other two harnesses.
There was a problem hiding this comment.
Reworked as asked, in all three harnesses. Each run now derives a unique RUN_ID and uses it for
both the workspace and the session name, and teardown deletes only the session and kernel ids that
run created -- the path-only match and the harness-name sweep are gone. The unique workspace also
removes the dead-kernel replay the sweep existed for, so no scan is needed at all. A prior crashed
run can no longer collide with a live one, and a person's session on the same server is untouched.
| "rayClusterSpec": { | ||
| "rayVersion": _ray_version(image), | ||
| "headGroupSpec": { | ||
| "rayStartParams": {"num-gpus": "0", "num-cpus": "0"}, |
There was a problem hiding this comment.
For a notebook that performs CUDA training in its own Python kernel, selecting a GPU profile does not give that kernel a GPU: the runner executes locally on the CPU-only head, while GPU requests exist only on workers. Such a notebook fails or falls back to CPU unless it already dispatches GPU work through Ray. Execute the notebook on GPU-bearing compute, or explicitly restrict submissions to notebooks that dispatch their GPU work through Ray.
There was a problem hiding this comment.
Fixed by refusing rather than faking it. The notebook executes in the control-only head, which
requests zero GPUs, so a gpusPerWorker > 0 profile is refused unless the request declares
gpuExecution=ray; the message names both ways forward (declare Ray dispatch, or pick a CPU
profile). The destinations catalog also reports notebookKernelGpu: false and requiresRayDispatch
so the picker states the limitation instead of implying the notebook gets a GPU. I took the
"restrict to notebooks that dispatch through Ray" option rather than moving the notebook onto
GPU-bearing compute, because the latter breaks the head-is-control-only contract the CLI renderer
shares.
| Only ready profiles are returned when a status block sets a non-empty state. | ||
| """ | ||
| spec = cluster.get("spec") or {} | ||
| raw = spec.get("workloadProfiles") or [] |
There was a problem hiding this comment.
TauCluster publishes resolved readiness under status.workloadProfiles.profiles, not status.state inside spec entries. Reading declarations here makes missing, stale or unready profiles selectable, and submission never checks their namespace applicability. Resolve the current-generation ready status catalog and enforce applicability for the selected namespace before listing or rendering a profile.
There was a problem hiding this comment.
Fixed in 4ad40e0. Readiness now comes from status.workloadProfiles.profiles -- those entries embed
WorkloadProfile inline, so the resolved catalog is self-sufficient and spec is only intent. A
profile missing or unready in the current generation is withheld with its reason attached; a
generation mismatch withholds the whole set rather than mixing generations. Applicability for
namespace, team and lane is enforced before a profile is offered and again before rendering, using
the same empty-means-global rule as core/resourceprofile, so an unknown team does not authorize a
team-scoped profile.
| if not hasattr(response, "read1"): | ||
| return response |
There was a problem hiding this comment.
With kubernetes==30.1.0 and urllib3==1.26.0, both allowed by the widgets extra, _preload_content=False returns an unread HTTPResponse without read1. Returning it unchanged makes namespace/run discovery empty, rejects an existing TauCluster during submission planning, and breaks RayJob status reads. Decode and close it with a bounded, deadline-preserving compatible reader, or require a verified urllib3 version with read1; the log/portal reader needs the same guarantee.
There was a problem hiding this comment.
Fixed in 5696058 — and this is a good catch, because a default environment cannot see it: it resolves urllib3 2.x, whose read1 present takes the other branch and hides the defect.
Root cause: read_document returned the response unchanged whenever the client had no read1. urllib3 1.26 has no read1, so callers received an unread HTTPResponse where they expected a mapping. bounded_body made the same assumption and is used directly by the pod-log reader and the portal series reader, so both were affected as you said.
_kube_ionow reads withread1when available and with a bounded, deadline-armedreadotherwise, and always decodes a readable response. Objects that are not readable at all still pass through unchanged, which is what test doubles return.tests/test_kube_io.pypins both generations, deadline arming through the modern timeout setter and the urllib3 1.26 socket path, the per-document ceiling, and the fail-closed case.
One correction that changed what I pinned: with kubernetes==30.1.0 and urllib3==1.26.0, the library does not merely lack read1 — it fails to import on Python 3.12+ with ModuleNotFoundError: urllib3.packages.six.moves. The declared floor is therefore now urllib3>=1.26.20 (the final 1.26 release): still a read1-less client, but installable across the Python versions we support.
Verified at that floor: 115 tests pass with kubernetes 30.1.0 and urllib3 1.26.20, and a real 1.26.20 HTTPResponse over a live socket decodes to a mapping through read_document.
I also added an SDK CI job that installs those exact floors, asserts the response class genuinely has no read1, and runs the affected suites, so a default environment cannot hide this again.
| try { | ||
| const snapshot = notebook.toJSON(); | ||
| setImports(possibleImports(snapshot)); | ||
| void session.current?.review({ notebook: snapshot, path: notebook.name, namespace, name: name.trim() || undefined, profile, queue, files: [...selected] }); |
There was a problem hiding this comment.
This request omits gpuExecution, so the server defaults to local and rejects every GPU profile, even when the notebook dispatches its GPU work through Ray. Add an explicit Ray-dispatch acknowledgement and carry it through preview and submission; retain the server-side refusal for local GPU execution.
There was a problem hiding this comment.
Fixed. The submit view now carries an explicit Ray-dispatch acknowledgement through preview and submission (gpuExecution: rayDispatch ? 'ray' : 'local'), shown next to the existing GPU warning whenever the chosen profile requests GPUs. The server-side refusal for local GPU execution is unchanged, so a request that does not acknowledge still gets refused rather than silently accepted.
The acknowledgement is part of the reviewed payload, so changing it invalidates the plan and forces a fresh review -- the digest covers it.
| run: | | ||
| python - <<'PY' | ||
| import importlib.metadata as metadata | ||
| for name, expected in (("kubernetes", "30.1.0"), ("urllib3", "1.26.0")): |
There was a problem hiding this comment.
The install step pins urllib3==1.26.20, but this assertion expects 1.26.0, so the floors job fails before running the regression tests. Required also depends on this job. Change the expected version to 1.26.20.
There was a problem hiding this comment.
Fixed. The assertion now expects 1.26.20, matching the pin. That was a straightforward miss when I moved the floor from 1.26.0 (which cannot be imported on Python 3.12+) to the final 1.26 release, and it would have failed the job before any regression test ran -- so it also defeated the point of adding the job.
| const response = await fetch(`${API}/api/sessions?token=${TOKEN}`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ kernel: { name: "python3" }, name: SESSION_NAME, path: NOTEBOOK, type: "notebook" }), |
There was a problem hiding this comment.
Jupyter's sessions POST reuses an existing session by notebook path and still returns 201; a unique name and workspace do not prove ownership. These IDs can belong to a user's active kernel, which this harness then executes in and deletes during teardown. Use a per-run notebook path, or refuse reused sessions before execution and cleanup. The embed and plugin harnesses need the same fix.
There was a problem hiding this comment.
Fixed in all three harnesses, and I went with the per-run path rather than only refusing reuse.
Each harness now copies the source notebook to examples/.tau-<name>-e2e-<RUN_ID>.ipynb through the Contents API, creates the session against that path, and asserts session.path === RUN_NOTEBOOK before executing anything. A session that does not carry this run's path is refused instead of adopted. Teardown deletes the copy along with the run's workspace.
I chose the unique path over pure refusal because refusal alone turns "someone has that example notebook open" into a failing test run, whereas a per-run path means no existing session can ever match. The ownership assertion is kept as a second line of defence in case a future Jupyter changes the reuse semantics again.
5142885 to
587326d
Compare
| // and still answers 201, so a unique session name and workspace do not prove | ||
| // ownership -- the returned id can belong to a person's live kernel, which this | ||
| // harness would then execute in and delete. Give every run its own notebook path. | ||
| const RUN_NOTEBOOK = `examples/.tau-plugin-e2e-${RUN_ID}.ipynb`; |
There was a problem hiding this comment.
Please share the ownership-sensitive notebook/session lifecycle across the button, embed, and plugin harnesses. All three copies currently derive RUN_NOTEBOOK before initializing RUN_ID, so they throw during module initialization before any browser checks run. A shared helper can initialize the identity first and preserve unique notebook paths and exact-ID cleanup, while keeping the browser journeys separate and covering the lifecycle with mocked API tests.
There was a problem hiding this comment.
Fixed, and the report was exactly right: all three harnesses threw at module load, before any browser step. const RUN_NOTEBOOK = ...${RUN_ID}... sat above const RUN_ID, and node --check cannot see a temporal-dead-zone error because it is a runtime fault, not a syntax one.
There is now one shared tools/notebook-e2e-lifecycle.mjs that owns the ownership-sensitive lifecycle: it initialises the run identity first, derives the per-run notebook path from it, seeds the copy through the Contents API, refuses a session whose path is not this run's, and tears down only exact ids. The three scripts keep their own browser journeys and delegate the lifecycle. tools/notebook-e2e-lifecycle.test.mjs covers it with 6 mocked-API tests that actually execute the lifecycle, so this class of bug cannot recur silently.
That gap is also now rule 1 in the required review discipline in AGENTS.md: syntax checks are not validation, and lifecycle logic must have a test that runs it.
| return read1(size) | ||
| read = getattr(response, "read", None) | ||
| if callable(read): | ||
| return read(size) |
There was a problem hiding this comment.
On the supported urllib3 1.26.20 path, read(size) can wait for the requested bytes across multiple socket receives. settimeout() limits inactivity, not the whole buffered read, so a slow-drip response can exceed the deadline before line 79 runs. This keeps Jupyter worker threads and the four metrics collection slots occupied after the advertised budgets expire. Use a transport path that enforces the absolute deadline, or require a verified read1-capable dependency, and cover this with a slow-drip transport regression.
There was a problem hiding this comment.
Fixed, and checking the capability rather than assuming it turned out to matter. urllib3 1.26 has no read1 on the response, but its _fp is an http.client.HTTPResponse, which has had read1 since Python 3.3. Verified against a real urllib3 1.26.20 response over a live socket:
urllib3 : 1.26.20
response.read1 : False
_fp type : HTTPResponse
_fp.read1 : True
_fp.read1(5) : b'{"met'
So _read_chunk now takes read1 from the response, else from _fp, and a response with neither is refused rather than read with a blocking read — which is the path you identified, since settimeout bounds inactivity and a drip resets it. That keeps the loop able to re-check the absolute deadline.
The slow-drip regression is in: a server drips 20 bytes at 0.2 s against a 1 s deadline, and the read must stop under 3 s rather than waiting the full 4 s for the block.
This also showed my earlier fakes were wrong — they modelled urllib3 1.26 as read-only, a shape no supported client produces, which is why the hazard looked handled. They now carry a real _fp.read1, with a separate NoRead1Body covering the refusal.
640e6fc to
b4bde29
Compare
| for candidate in (getattr(response, "read1", None), | ||
| getattr(getattr(response, "_fp", None), "read1", None)): | ||
| if callable(candidate): | ||
| return candidate(size) |
There was a problem hiding this comment.
A chunked response can still outlive the absolute deadline: CPython reads the chunk-size line and trailers with buffered readline() before read1() returns. A peer dripping a chunk extension avoids the socket inactivity timeout, so control never reaches the deadline check; four such metrics reads can hold every collection slot. Enforce a deadline that interrupts framing as well as body reads, and add a chunked slow-drip regression that verifies the worker and slot are released.
There was a problem hiding this comment.
Right, and this is the third layer of the same problem. Rounds one and two bounded the body read; neither bounded the framing. http.client parses the chunk-size line and trailers with buffered readline() before read1 is ever reached, so a peer dripping a chunk extension never re-enters our deadline loop and never trips the inactivity timeout.
Fixed by enforcing the absolute deadline rather than the inactivity one. A _DeadlineWatchdog timer thread is armed at the caller's deadline and shutdown(SHUT_RDWR)s the underlying socket and closes the response, which interrupts the buffered readline() mid-framing. bounded_body maps that teardown to TimeoutError and re-checks the deadline before reporting success, so a torn-down read cannot be reported as a good one.
Regression: test_chunked_slow_drip_interrupts_framing_and_releases_workers runs a real Transfer-Encoding: chunked server dripping an unterminated ;drip chunk-extension every 0.05s to four concurrent readers holding a BoundedSemaphore(4) with 1s deadlines. It asserts all four raise TimeoutError, elapsed is >=1s and <4s, and all four slots are re-acquirable - i.e. the worker and the slot are both released, which is the part you asked about. Observed 1.54s.
The control matters here: with the watchdog stubbed to a no-op, the same single drip was still blocked after 5s against a 1s deadline. That is the failure you described, reproduced, and then closed.
| sock.settimeout(min(30, remaining)) | ||
| elif not response.isclosed(): | ||
| raise TimeoutError("Cannot enforce response read deadline") | ||
| chunk = response.read1(min(4096, API_LIMIT + 1 - len(data))) |
There was a problem hiding this comment.
This separate response loop assumes read1 exists on the urllib3 response, but the supported 1.26.20 floor lacks it. The shared SDK reader handles the underlying _fp.read1 path, while demo tests replace call, so the demo fails on its first API response despite the floor tests passing. Reuse the shared bounded reader, preserving the byte ceiling, deadline, cleanup and no-retry policy, and cover the real call path with a legacy-shaped response.
There was a problem hiding this comment.
Correct, and this one is my own rule violated: I fixed the shared reader and never grepped for other consumers of the same pattern, having just written "a duplicated implementation that must agree" into the review discipline.
The demo's private loop is deleted; it now calls bounded_body(response, deadline, API_LIMIT) with the status captured first, preserving the byte ceiling, the deadline, response.close() and retries=False. Production .read1( call sites are now zero - the only read1 resolution left is tau/_kube_io.py::_read_chunk. Everything else routes through the shared reader: tau/jupyter/metrics.py:92, tau/jupyter/runs.py:534, and tau/jupyter/destinations.py, tau/_backend.py, tau/_notebook_submit.py via read_document. The preload_content=False sites are the demo, metrics.py:87, runs.py:533 and _kube_io.py:161, and all four feed it.
Coverage now exercises the real call() against a LegacyPoolManager returning a response with no response.read1 but a working _fp.read1, plus a ceiling/cleanup case, instead of replacing call as the old test did.
| // channel, so no bundled Chromium is used. | ||
| const PROFILE_DIR = | ||
| process.env.PLAYWRIGHT_PROFILE_DIR || | ||
| path.join(process.env.LOCALAPPDATA || process.env.TEMP, "tau-jupyter-work-profile"); |
There was a problem hiding this comment.
Importing this lifecycle module throws when PLAYWRIGHT_PROFILE_DIR, LOCALAPPDATA and TEMP are absent: browser-only initialization passes undefined to path.join. That prevents the documented offline lifecycle suite from running on an otherwise valid Linux/macOS setup, although it never launches a browser. Resolve the profile inside launchWorkProfileEdge, retain the override precedence and use os.tmpdir() as the final fallback; test importing with those variables absent.
There was a problem hiding this comment.
Fixed. Top-level PROFILE_DIR is gone; workProfileDir() resolves PLAYWRIGHT_PROFILE_DIR -> LOCALAPPDATA -> os.tmpdir() inside launchWorkProfileEdge, so importing the module no longer touches the environment at all.
Verified directly: node --input-type=module -e "await import('./tools/notebook-e2e-lifecycle.mjs')" with PLAYWRIGHT_PROFILE_DIR, LOCALAPPDATA and TEMP unset prints import-ok. The test suite spawns a child with those three cleared (and NODE_TEST_CONTEXT stripped so the nested runner reports to stdout) and asserts exit 0 with the five mocked-API lifecycle tests still passing. A separate test pins the override precedence so the fallback cannot silently take over.
This is the same class as the earlier RUN_ID temporal-dead-zone defect - a module top level that throws on import - so "imports cleanly under a minimal environment" is now part of what the lifecycle module is expected to guarantee.
ee76605 to
4cf39ec
Compare
| namespaces, teams, and lanes; an empty set means global, and a non-empty set must | ||
| contain the caller's value. The selected namespace is always known, while team | ||
| and lane are supplied when the caller knows them. Because an unknown scope value | ||
| does not authorize a scoped profile, team-scoped profiles are withheld when no |
There was a problem hiding this comment.
Unknown team/lane values are not filtered by ready_profiles, and build_plan derives them from the selected profile; the tests explicitly preserve that behavior. This paragraph instead promises an authorization boundary the notebook does not enforce. Document the actual selection policy here and in the resolver docstrings, including that it does not authenticate the caller's team, without changing the existing behavior.
There was a problem hiding this comment.
Corrected. The paragraph promised an authorization boundary the notebook did not enforce, and it was worse than vague - it was specifically false for the submit path.
What the document now says, and what the code now does:
- team and lane are NOT user input and are NOT derived from the profile. They come from the Jupyter server's operator-set environment (
TAUGRID_TEAM/TAUGRID_LANE); a request body carrying either is refused 400. - an unknown caller scope fails closed: a team-scoped profile is withheld rather than admitted by an unfiltered check.
- a namespace-scoped profile is only usable for a namespace the server can actually list, and the per-namespace grant is checked before that namespace's profiles or LocalQueues are read - not after.
- the boundary does NOT authenticate an individual caller's team. Cluster access runs under the server service account, so a deployment serving several teams must run one server per team.
Two of those three were added after your review, because an independent pass found the same class twice more: first the namespace was still taken from the request, then a request that OMITTED the namespace reached discovery and read the selected namespace's queues holding only the coarse taugrid grant. Both are now covered by handler-level tests with a denying Authorizer.
So the sentence is now true, and where it is not - per-caller team identity - the document says so instead of implying it.
dfc1d81 to
e20d05a
Compare
|
Hey sorry I'll take a look at this was busy with a few other items. Will check on it tomorrow |
| ) | ||
| lines.append("cd " + DURABLE_ROOT) | ||
| lines.append(base) | ||
| return "\n".join(lines) + "\n" |
There was a problem hiding this comment.
Native submissions use K8sJobMode, but KubeRay 1.6.2 inserts this multiline entrypoint verbatim into the submitter shell command. The newline after set -eu ends ray job submit, so dependency installation and the notebook runner execute in the submitter rather than the Ray head. That pod has only 1 CPU/1Gi and cannot discover the intended cluster with ray.init(address="auto"). Return a safely quoted shell invocation containing the complete script, and cover the actual submitter command interpretation.
There was a problem hiding this comment.
Fixed, and verified against a real POSIX shell rather than against the Python string.
$ Select-String -Path sdk/python/python/tau/_render.py -Pattern 'shlex.quote' | Measure-Object
Count: 3
For K8sJobMode, _entrypoint(..., submitter_shell=True) returns bash -lc <shlex.quote(script)>. The earlier form was inserted verbatim into KubeRay's submitter shell command, where the newline after set -eu terminated ray job submit and the rest ran in the submitter - a pod with 1 CPU / 1Gi that cannot discover the cluster.
Evidence, in a Linux container with ray 2.56.0's own shlex.join (ray/dashboard/modules/job/cli.py:313-317) and faked ray/python3 that echo their argv: for a hostile script containing a single quote, a newline, $(...), a backtick and #, the argv after -- is ["bash","-lc",<whole script>], the script arrives intact as ONE argument, and nothing executes in the submitter. On the head, $(...) and the backtick execute there, as intended.
The test asserts argv[6:8] == ["bash","-lc"] and len(argv) == 9; it is POSIX-only and skipped on Windows, and it also corrected a second error the reviewer had not flagged - the old assertion expected the framed argument to start with python3 /script/_tau_runner.py when it actually starts with set -eu, so that test had never passed on any platform.
Worth recording what is NOT proven here: KubeRay's own interpolation is modelled from ray's CLI source and this repo's e2e assumption, not executed against a live KubeRay, because no reachable cluster with the TauGrid CRDs was available.
Kevin Cho (chokevin)
left a comment
Kevin Cho (chokevin)
left a comment
There was a problem hiding this comment.
Notebook support builds, packages, registers, and passes its offline Python and JavaScript suites. The inline comments identify reproduced correctness issues and unrelated scope that should be addressed before merge. Live Kubernetes, browser, and runtime-image execution remain unverified.
| @@ -1 +1 @@ | |||
| 1.26.7 | |||
| 1.26.9 | |||
There was a problem hiding this comment.
Can we remove the Go toolchain and golang.org/x/net upgrades from this PR? The same dependency bump is repeated across every Go module, but the notebook implementation is in the Python SDK and does not depend on it. This repository-wide maintenance makes the notebook diff harder to validate and should be handled in a separate dependency PR.
There was a problem hiding this comment.
Removed from this PR - and main has since done exactly what you asked, so there is nothing left to split.
$ git diff --name-only origin/main..HEAD -- .go-version cli/go.mod cli/go.sum core/go.mod core/go.sum portal/go.mod portal/go.sum controllers/tau-core/go.mod tests/e2e/go.mod
(empty)
$ git show HEAD:.go-version ; git show origin/main:.go-version
1.26.9
1.26.9
The branch was rebased onto main, where .go-version 1.26.9 and golang.org/x/net v0.60.0 already landed independently. Both sides are now byte-identical, so the dependency bump is no longer part of this PR's diff at all. This PR no longer carries a dependency-version change.
I should be explicit that I overruled this request when you first made it, on the grounds that reverting .go-version here re-armed GO-2026-6617/6613 and turned the seven Go jobs red. That was true at the time and false within days: the cost of keeping it was a branch that fell 30 commits behind and conflicted across 18 files on this one change. You were right on the merits.
| # conditions must key on evidence, doc citations must resolve, and no | ||
| # harness screenshot or Jupyter checkpoint may be tracked. | ||
| - name: Check the mechanically detectable review patterns | ||
| run: python3 scripts/ci/check-review-patterns.py |
There was a problem hiding this comment.
Please split the repository-wide review-pattern framework out of the notebook PR. This job introduces a 931-line checker and new global requirements through the root Makefile, AGENTS.md, DEVELOPMENT.md, and docs/TESTING.md. The notebook lifecycle test on line 60 belongs here, but changing review policy and required validation for every TauGrid subsystem is an independent initiative with a much larger blast radius.
There was a problem hiding this comment.
Agreed, and here is the split I would make. The sequencing constraint matters, so please read the last paragraph before deleting anything.
Keep in this PR (notebook-specific):
- the notebook E2E lifecycle test in the notebook job
- the
notebook-runtimeimage job (docker buildofimages/notebook-runtime/Dockerfileplus its import check) - the notebook execution path depends on that image, so it belongs with the feature
$ Select-String -Path .github/workflows/taugrid-validation.yml -Pattern 'notebook-e2e-lifecycle' | Measure-Object
Count: 1
$ Select-String -Path .github/workflows/taugrid-validation.yml -Pattern 'notebook-runtime' | Measure-Object
Count: 9
Move to a follow-up PR (repo-wide):
scripts/ci/check-review-patterns.py(1,888 lines, 18 checks plus--self-testand--scope-report)- its
review-patternsCI job (5 references in the workflow) - the policy text in
AGENTS.md,DEVELOPMENT.mdanddocs/TESTING.md
The constraint: please land the follow-up first, or keep the checker here. Do not simply delete it. This round it caught nine real defects in this very diff - the circular scope derivation, the unguarded handlers, the status.failed classifier, the discarded profile queue, the leaked staging directory, the incomplete metadata contract, the unframed entrypoint, the unbuilt image, and the namespace-authorized-by-request bug. Those are C1-C7 and the namespace fix. Removing the checker here leaves them mechanically unguarded until the follow-up lands, and the diff would then be relying on the reviewers who already missed two instances of the same authorization class.
Happy to do either: split it now with the checker staying until the follow-up is in, or leave the split entirely to a follow-up PR. Your call on the sequencing - I did not want to silently delete a guard that is currently doing work.
There was a problem hiding this comment.
Correction to my reply above, because the number mattered and it was wrong.
I wrote that the checker "caught nine real defects in this very diff" and then listed nine items. It caught eight. The ninth item in my list - the namespace authorized by a request that omits it, so discovery reads a namespace's LocalQueues holding only the coarse grant - was found by a reviewer, not by check-review-patterns.py. Attributing it to the checker overstates what the guard actually does, which is the one thing I should not do while arguing the guard is worth keeping.
The eight it does guard are the checks named for them: authorization-input-provenance, handler-authorization-required, terminal-state-from-current-state, resolved-value-not-discarded, staging-resource-disposal, run-metadata-contract-complete, entrypoint-shell-framed and images-built-in-ci.
The argument for keeping the checker is unchanged - eight mechanically enforced is still eight more than the reviewers caught unaided, and this PR is the evidence for that - but the count should have been one lower and I should have checked it before writing it rather than after.
| // submitterConfig.backoffLimit. Unset lets KubeRay apply its own default | ||
| // (2), which retries a deterministic failure; the successful retry then | ||
| // masks the real failure in the RayJob status. | ||
| "submitterConfig": map[string]any{"backoffLimit": int64(0)}, |
There was a problem hiding this comment.
This changes retry behavior for every RayJob rendered by the Go CLI, not only notebook submissions. The notebook renderer already sets its own submitterConfig, so notebook support does not require this CLI behavior change. Please move this to a separate PR with dedicated CLI tests, or remove it from this one.
There was a problem hiding this comment.
Reverted - this PR no longer changes CLI retry behaviour.
$ Get-ChildItem cli -Recurse -Include *.go | Select-String 'submitterConfig' | Measure-Object
Count: 0
cli/internal/rayjobrender/render.go matches origin/main for that hunk; the two unrelated RDMA hunks in the same file are pre-existing on this branch and were left alone. No Go test asserted the pinned value, so nothing else moved.
The CLI now behaves as it does on main, which means KubeRay applies its own default submitter retry (2). Pinning it belongs in its own PR with dedicated CLI tests, since it changes retry semantics for every RayJob the Go CLI renders - it is deferred deliberately, not dropped. Worth knowing: the retried-success defect is therefore unguarded for CLI-rendered jobs until that follow-up lands.
| repository: oci://mcr.microsoft.com/aks/ai-runtime/helm | ||
| version: 0.1.8 | ||
| digest: sha256:0bab13fd42edbd747e5d5f1050f00c63bf7662a76726c34a78c3d9e5c7aca8f9 | ||
| generated: "2026-10-08T16:34:55.1944882+08:00" |
There was a problem hiding this comment.
Please remove this generated lock file from the notebook PR. There is no accompanying notebook chart integration or chart dependency change, so committing a new repository-level dependency lock is unrelated churn and can alter Helm dependency resolution outside this feature.
There was a problem hiding this comment.
Removed. This branch added it accidentally.
$ git ls-files charts/taugrid/Chart.lock # at HEAD
(empty)
$ git ls-tree -r --name-only origin/main -- charts/taugrid/Chart.lock
(empty)
$ git check-ignore -q charts/taugrid/Chart.lock && echo ignored
ignored
Deleted from the tree and added to .gitignore, which restores main's state. The repo's own gpu-ray-tune.md already describes it as an untracked generated artifact, so this aligns the tracking with the documented intent. No test referenced it.
| )) | ||
| if scope.team is not None: | ||
| checks.append(( | ||
| "team", |
There was a problem hiding this comment.
This authorizes team-scoped capacity without a trusted caller team. A minimal repro produced unknown_team_catalog_profiles=['team-only'] and then successfully built a plan for that same profile by deriving team alpha from the profile being authorized. Any namespace user can therefore select capacity reserved for another team. Unknown caller scope must fail closed for scoped profiles; resolve team identity from a trusted server-side source and use the same identity for discovery and submission. Add a handler-level test proving a request without a matching trusted team can neither list nor submit the profile.
There was a problem hiding this comment.
Fixed, and hardened twice since you reviewed it.
The caller's scope is no longer read out of the object being authorized. Previously build_plan derived the team/lane FROM the selected profile and used that to authorize the profile - a circular check that authorizes everything. That derivation is gone; one ProfileScope from a trusted source is used for both discovery and the decision.
Trusted source: the Jupyter server's own process environment (TAUGRID_TEAM/TAUGRID_LANE, set by the operator at server start). A notebook author cannot set a server env var from a request, and a request body that carries team or lane is refused with 400.
A namespace-scoped profile is now only usable for a namespace the server can actually list. An independent review then found the same class a third time - a request that OMITS namespace reached discovery and read the selected namespace's LocalQueues after authorizing only the coarse taugrid resource - so the per-namespace decision is now made before profiles or queues are read.
Handler-level negative controls, not resolver unit tests:
- team scoped, team unset or non-matching -> preview 400 AND
api.created == [] - namespace scoped, namespace not in the server's visible list -> 403; same request with the namespace visible -> plan returned
- request body carrying team/lane -> 400
Honest limit: this authenticates the SERVER, not the individual caller. Cluster access runs under the server service account, and under the stock allow_all Authorizer any authenticated caller may target any namespace the server can see. A deployment serving several teams must run one server per team. That is documented rather than implied.
The stock-install consequence is deliberate: team-scoped capacity is withheld and the picker is empty for it. I chose that over weakening the check back to unfiltered.
| except NoTauClusterFound as exc: | ||
| raise SubmitError(str(exc), status=409) from exc | ||
| except SubmitError: | ||
| raise |
There was a problem hiding this comment.
Submission discards the selected profile’s queue routing. The repro catalog resolved namespace-q from the profile’s localQueues, while build_plan() rendered workspace-q. This can send GPU workloads to the wrong quota path or leave them unadmitted. Resolve the effective queue in canonical order: profile namespace mapping, profile default, then workspace fallback. Add a test where all three values differ and assert both preview and rendered metadata use namespace-q.
There was a problem hiding this comment.
Fixed, and since your review the profile's resolved queue is canonical rather than merely preferred.
Order: profile namespace LocalQueue -> profile defaultLocalQueue -> workspace default -> default. Prior to this, the request's queue was applied AFTER resolution, so queue or profile_queue(...) let a caller name another team's queue - and the queue is the quota-bearing object, which defeats the point of the team-scoped profile. An independent review caught that; a request naming a queue other than the profile's is now refused 400 and names the profile's queue.
$ Select-String -Path sdk/python/python/tau/_notebook_submit.py -Pattern 'resolved_queue|profile_queue' | Measure-Object
Count: 5
Tests: a case where namespace mapping, profile default and workspace default all differ asserts namespace-q in the plan, the preview summary and the rendered kueue.x-k8s.io/queue-name label; plus a case for profile default when no namespace mapping matches. The destinations catalog row, the plan and defaultQueue all resolve the same profile value.
| def _default_staging_dir(self) -> Any: | ||
| import tempfile | ||
|
|
||
| return Path(tempfile.mkdtemp(prefix="tau-notebook-")) |
There was a problem hiding this comment.
The default staging directory leaks notebook contents. A successful fake submission created one workload, but the generated directory still existed after both submission and panel.close(). Repeated use on a long-running Jupyter server will retain user notebooks and consume disk. Track whether the panel owns the temporary directory, remove it on failure and before replacement, and clean it in close(). Add default-path lifecycle tests rather than supplying a test-managed temporary directory.
There was a problem hiding this comment.
Fixed. The panel now owns the staging directory it creates and releases it in every exit path.
$ Select-String -Path sdk/python/python/tau/widgets/panel.py -Pattern '_discard_owned_staging_dir|_new_owned_staging_dir' | Measure-Object
Count: 6
close() removes it; submission removes it on failure and before creating a replacement; and a caller-supplied staging_dir is never touched, because the panel only tracks directories it created itself.
Tests use the DEFAULT path deliberately, not a test-managed temporary directory - the leak was in the default path, so supplying one would have tested nothing:
- close() releases it
- a failed submission releases it
- replacing a submission releases the previous one
- a caller-supplied directory survives all of the above.
| metadata: Dict[str, Any] = { | ||
| "name": name, | ||
| "namespace": namespace, | ||
| "labels": {TAU_QUEUE_LABEL: queue, TAU_MANAGED_BY_LABEL: TAU_MANAGED_BY_VALUE}, |
There was a problem hiding this comment.
This only partially implements TauGrid’s run identity contract. managed-by makes the RayJob appear in tau run list, and exact-name status/log reads can find it, but the resulting list row has an empty run ID and profile/provenance consumers receive incomplete metadata because tau.azure.com/run-id, tau.azure.com/workload-kind, and resolved profile metadata are absent. Stamp the shared workload metadata contract and add a compatibility test that sends the rendered object through the existing core/runs parser and status/profile consumers, not only the notebook-specific reader.
There was a problem hiding this comment.
Fixed, and the compatibility check is cross-language rather than notebook-specific.
$ Select-String -Path sdk/python/python/tau/_render.py -Pattern 'RUN_ID_LABEL|WORKLOAD_KIND_LABEL' | Measure-Object
Count: 6
$ ls core/runs/notebook_compat_test.go core/status/notebook_compat_test.go
both present
The renderer stamps managed-by plus tau.azure.com/run-id (defaulting to the object name), tau.azure.com/workload-kind, the resolved profile, and team/lane when known, plus provenance annotations (workload-profile, tau-cluster-generation from the observed generation, workload-profile-set-hash). An invalid label value raises rather than being truncated, since shortening an identity value silently is how two runs collide.
The compatibility test runs through the REAL shared consumers, not a notebook-only reader: core/testdata/notebook-rayjob.json is pinned by the Python test and parsed by core/runs/notebook_compat_test.go (parseRayJobs -> RunID/Kind/Queue) and core/status/notebook_compat_test.go (hydrateRayJob -> RunProfileFields run_id/workload_kind/profile/team). Negative control: removing run-id from the renderer makes the Go test fail with RunID=""; removing profile/team yields not declared.
|
|
||
| COPY wheels/ /wheels/ | ||
|
|
||
| RUN if ls /wheels/*.whl >/dev/null 2>&1; then \ |
There was a problem hiding this comment.
The notebook execution path depends on this image, but this PR does not build it in CI. The Python and frontend suites pass without proving that the base image can install/import the executor stack or run the packaged notebook. Please add a no-push image smoke build to notebook validation (including the final import check), with a deterministic wheel-preparation step or documented network dependency. Until that exists, installation is verified but actual notebook execution remains unverified.
There was a problem hiding this comment.
Added, plus a startup gate so a misconfigured deployment fails early rather than at job submission.
$ Select-String -Path .github/workflows/taugrid-validation.yml -Pattern 'notebook-runtime' | Measure-Object
Count: 9
The notebook-runtime job does a no-push docker build of images/notebook-runtime/Dockerfile and then runs the executor's own import check (python3 -c "import nbformat, nbconvert, ipykernel"), wired into the required results. Verified locally: build succeeded and the import check printed notebook executor ready.
The gate: an independent review found that submission could be enabled while the renderer still pointed at the uncertified ray base image, whose python3 -c "import nbformat" fails with ModuleNotFoundError. server.py now calls submission_config_error() first in _load_jupyter_server_extension, so enabling submission without TAUGRID_RUNTIME_IMAGE fails at extension load with a clear message. I did not make the certified image the default because it is not published to MCR yet - hardcoding the tag would move the trap rather than close it. Startup test added.
| # Ray inner job finished while the submitter (which runs the notebook) did | ||
| # not. With renderers pinning submitterConfig.backoffLimit to 0 there is no | ||
| # retry, so any failed submitter attempt means the submission failed. | ||
| if isinstance(status.get("failed"), int) and status["failed"] > 0: |
There was a problem hiding this comment.
KubeRay’s status.failed counts failed RayJob attempts, not terminal submitter failure. With spec.backoffLimit > 0, it stays positive while a retry runs and after that retry succeeds. This branch therefore reports those existing runs as terminal failed and stops native and legacy watching. Use the current deployment/execution lifecycle for terminal state and cover a failed attempt followed by RUNNING and SUCCEEDED/Complete.
There was a problem hiding this comment.
Fixed. status.failed is no longer read anywhere in the classifier.
It is a count of failed RayJob ATTEMPTS, not a terminal-failure signal: with spec.backoffLimit > 0 it stays positive while a retry runs and after that retry succeeds. Using it as a terminal predicate reported healthy runs as failed and stopped native and legacy watching.
Terminal state now comes from the current lifecycle - jobStatus, jobDeploymentStatus and reason - before the running/complete branches.
There is a consequence worth stating, because an independent review caught me getting it wrong once already: with the counter gone I briefly classified jobStatus=SUCCEEDED plus KubeRay's "Job submission has failed" message as complete, and a test encoded that. The submitter-failed message is now folded into the failed disjunction, so that case reports failed without reading the counter. It is also caught by jobDeploymentStatus == "Failed", which is what tests/e2e/k8s.go assumes - but the message backstop does not depend on the deployment field being populated.
Coverage: failed -> RUNNING, failed -> SUCCEEDED/Complete, jobDeploymentStatus=Failed + jobStatus=SUCCEEDED, and jobStatus=SUCCEEDED + submitter-failed message with no deployment status.
|
|
||
|
|
||
| class SubmitHandler(APIHandler, _SubmitMixin): | ||
| @web.authenticated |
There was a problem hiding this comment.
On a Jupyter Server with a restrictive Authorizer, @web.authenticated establishes identity but does not check permission; APIHandler.prepare() does not call is_authorized either. An authenticated user denied execution can therefore submit workloads under the server’s Kubernetes identity when submission is enabled. Enforce explicit Jupyter authorization actions/resources before these operations, including protected reads, and add handler-level tests with a denying Authorizer.
There was a problem hiding this comment.
Fixed. Every handler that reads or writes cluster state now consults the Jupyter Authorizer.
$ Select-String -Path sdk/python/python/tau/jupyter/server.py -Pattern 'self\.authorize\(' | Measure-Object
Count: 10
_ClientMixin.authorize(action) calls authorizer.is_authorized(self, current_user, action, "taugrid:<namespace>"), awaits the result when it is awaitable, and fails closed on a missing Authorizer or user. Handlers call it with read for capabilities/destinations/status/runs/files/namespaces/logs/preview and execute for submit, so a read-only grant cannot trigger a submission. The resource is namespace-qualified rather than the single coarse taugrid string - though note it is a policy hook, not an authenticator: under the stock allow_all it grants everything.
Tests drive all nine handlers with a denying Authorizer (403, authorizer consulted, no cluster calls), plus read-vs-execute and an anonymous case. Handler-level with a real Authorizer, not a stub of our own method.
e20d05a to
98317c7
Compare
| from tau.jupyter.destinations import list_destinations | ||
|
|
||
| namespace = self.get_argument("namespace", None) | ||
| await self.authorize("read", namespace=namespace) |
There was a problem hiding this comment.
When an Authorizer allows discovery but denies reads in the selected namespace, omitting namespace bypasses that denial: discovery selects a namespace and reads its LocalQueues after authorizing only taugrid. Authorize taugrid:<resolved namespace> before resolving its profiles or reading its queues, and cover the initial request with a namespace-denying Authorizer.
There was a problem hiding this comment.
Fixed in cec2e5b.
I verified before claiming it: this was not fixed at 4993c0a. A /destinations request that omitted namespace authorized only the coarse taugrid resource, then list_destinations resolved namespaces[0] and read that namespace's LocalQueues.
Negative control on the pre-fix tree (4993c0a, scratch worktree), with an Authorizer that grants everything except taugrid:research:
$ python neg.py
authorizer calls: [('read', 'taugrid')]
client calls: ['namespaces', 'clusters', 'queues']
result namespace: research queues: ['research-gpu']
Only taugrid was authorized, yet research's queues were listed. The same script on the fixed tree:
$ python neg_fixed.py
...
tau/jupyter/server.py:404: in get
await self.authorize("read", namespace=resolved)
...
tornado.web.HTTPError: HTTP 403: Forbidden (not authorized to read TauGrid notebook resources)
The fix:
sdk/python/python/tau/jupyter/server.py:389DestinationsHandler.getauthorizes the coarse grant for the cluster-wide namespace listing, resolves the namespace, then authorizestaugrid:<resolved>atserver.py:404beforelist_destinationsreads profiles/queues.sdk/python/python/tau/jupyter/destinations.py:34resolve_destination_namespaceis the resolution;destinations.py:14_pick_namespacestill refuses a namespace outside the visible list (ReadError), so the request value never authorizes itself.
Regression test tests/test_jupyter_destinations.py::test_omitted_namespace_is_authorized_as_the_resolved_namespace asserts a 403 and that no queue was listed when taugrid:research is denied; the success-path test asserts ("read", "taugrid:research") was authorized.
$ python -m pytest tests/test_jupyter_destinations.py tests/test_jupyter_contract.py tests/test_jupyter_runs.py tests/test_jupyter_submit.py -q
119 passed in 3.96s
| " stream.write(content.get('text', ''))\n" | ||
| " stream.flush()\n" | ||
| " return super().process_message(msg, cell, cell_index)\n" | ||
| " ep = StreamingExecutor(timeout=600)\n" |
There was a problem hiding this comment.
A Python notebook saved with a local environment’s kernelspec name is accepted, but this executor requests that same name in the remote image. If the image only has its standard Python kernel, the submitted job fails before executing any cell. Select the remote Python kernel explicitly, or reject unsupported names during preview, and add a named-environment regression.
There was a problem hiding this comment.
Fixed. The reviewer is right, and this was a real end-to-end break, not a theoretical one.
StreamingExecutor(timeout=600) passed no kernel_name, and nbconvert's ExecutePreprocessor then honours the notebook's own metadata.kernelspec.name. A notebook saved against a local virtualenv therefore asked the runtime image for a kernel it does not have, and the run failed with NoSuchKernel before any cell executed.
The generated runner now pins it:
line 7 of the generated _tau_runner.py : REMOTE_KERNEL_NAME = 'python3'
line 50 : ep = StreamingExecutor(timeout=600, kernel_name=REMOTE_KERNEL_NAME)
Verified against the pre-fix runner in a scratch tree: with kernel_name reverted the regression test fails; with the fix it passes. So it is a control, not a restatement of the implementation.
Two things worth stating rather than glossing:
-
Pinning the kernel changed the generated runner, which changes the payload digest in the rendered manifest - so the pinned compatibility fixture
core/testdata/notebook-rayjob.jsonhad to be regenerated. That is the fixture doing its job. Both Go consumers still accept it:go test ./runs/... ./status/... -run Notebookpasses, so the cross-language contract holds. -
python3is the image's standard kernel name, chosen fromimages/notebook-runtime/Dockerfileinstallingipykernel. I have not executed it inside that image on this host, so if the image registers the kernel under a different name the pin needs to change with it. Flagging it as an inference rather than tested fact.
There was a problem hiding this comment.
Fixed. This was a real defect, not a false alarm.
Root cause, confirmed by reading the third-party resolution path rather than assuming it. nbclient.client.NotebookClient.create_kernel_manager:
if not self.kernel_name:
kn = self.nb.metadata.get("kernelspec", {}).get("name")
if kn is not None:
self.kernel_name = kn
So with no kernel_name, ExecutePreprocessor asks for the notebook's own metadata.kernelspec.name. The runner previously built StreamingExecutor(timeout=600), so a notebook saved against a local virtualenv asked the pod for a kernel the image does not have.
Negative control on the pre-fix tree (4993c0a), executing the real generated _tau_runner.py with a notebook whose metadata.kernelspec.name is my-local-env:
runner exit code : 1
jupyter_client.kernelspec.NoSuchKernel: No such kernel named my-local-env
Same notebook on the fixed tree (d6c8eab):
runner exit code : 0
tg-runner: executing analysis.ipynb from .../stage
hello from the cluster
tg-runner: wrote .../data/analysis.executed.ipynb
Fix and where:
sdk/python/python/tau/_notebook_pkg.py:178the runner now carriesREMOTE_KERNEL_NAME = 'python3'.sdk/python/python/tau/_notebook_pkg.py:221ep = StreamingExecutor(timeout=600, kernel_name=REMOTE_KERNEL_NAME)— an explicitkernel_namewins over the notebook's metadata in the resolution path above.
python3 is the right name in the shipped image, not just locally. images/notebook-runtime/Dockerfile:23-29 installs ipykernel, and I built that image and asked it:
$ docker build -t taugrid-notebook-runtime:kernelcheck images/notebook-runtime
$ docker run --rm --entrypoint python3 taugrid-notebook-runtime:kernelcheck \
-c "from jupyter_client.kernelspec import KernelSpecManager; print(sorted(KernelSpecManager().find_kernel_specs()))"
kernel names: ['python3']
The image exposes exactly one kernelspec, named python3, so no further change is needed there.
Regression test sdk/python/python/tests/test_notebook_runner_kernel.py runs the real generated runner in a subprocess (nbconvert stubbed so it does not depend on the test env's notebook stack). It is a control, not a restatement: against the pre-fix runner it observes my-local-env and fails; against this branch it observes python3 and passes. The stub mirrors nbclient's resolution order, so a different valid fix (rewriting the metadata) would also pass.
$ python -m pytest tests/test_notebook_runner_kernel.py -q # on 4993c0a
E AssertionError: assert 'my-local-env' == 'python3'
$ python -m pytest tests/test_notebook_runner_kernel.py -q # on d6c8eab
1 passed
Sweep: nbconvert is only reached through the generated runner, so kernel_name in _runner_script() is the single place the request is made — there is no second copy to fix.
Suite state on this head: ruff check . clean, scripts/ci/check-review-patterns.py "All 18 review-pattern checks passed", --self-test "18 checks; every fixture defect failed its check", license headers pass. python -m pytest is 54 failed / 356 passed here, and all 54 failures are the pre-existing Windows-only WinError 193 failures in test_cluster_wrapper.py/test_workloads.py/test_eval.py/test_stellar.py/test_build.py (they execute a Unix shell script as a program); the untouched base fails in exactly the same five files.
Pushed as a5843af (fix) + d6c8eab (test) on feat/notebook-plugin.
JupyterLab runs browser, run detail, logs and loss surfaces, plus a toolbar submit flow that turns the open notebook into a TauGrid RayJob. The wheel ships the prebuilt labextension and enables the server extension on install. Submission resolves the current-generation ready status catalog from status.workloadProfiles.profiles, enforces applicability for the namespace the run lands in, and refuses a GPU profile unless the request declares that the notebook dispatches its GPU work through Ray. The submit view carries that acknowledgement. Status is derived once, in describe_run/explain_run, and read by the runs browser, the detail panel and the log surface, so one run cannot be explained two ways. A failed submission carries the run's own reason, an explanation, an actionable suggestion and a run-failed diagnostic; the execution phase reports failed rather than warning; each phase key appears once; pod readiness no longer reads as success for a failed run; and the failing log is reachable. Verified end to end on a live cluster, including all three browser harnesses against real Edge. Review fixes folded in here: - Readiness comes from the resolved status catalog, not a declaration. - Bounded reads work on the supported dependency floor: read1 when present, otherwise read1 from the underlying http.client response, which is what urllib3 1.26 provides; a response with neither is refused rather than read unbounded. A slow-drip regression pins the absolute deadline. - The submitter Job no longer retries a deterministic failure: both renderers pin submitterConfig.backoffLimit, which KubeRay 1.6 moved off spec.backoffLimit, and status.failed plus KubeRay's submission-failed message classify as failure because jobStatus can still read SUCCEEDED. - The browser harnesses share one notebook/session lifecycle that initialises the run identity before deriving the per-run notebook path, copies to a non-hidden name (Jupyter's contents API rejects hidden names with 400), refuses a session it does not own, and deletes only exact ids. A mocked test executes the lifecycle. - Assertions key on evidence of success rather than on a token an error message also contains. run-embed-e2e asserted /TauGrid/ while the proxy's own banner reads "TauGrid portal proxy could not reach ...", so it passed on an error page; it now requires the run name and rejects transport-failure text. run-button-e2e accepted a repaint while the panel reported `unknown`; the recording fake now seeds a queued status like the real path and the harness asserts it. - The generated runner honours TAU_DATA_DIR instead of hard-coding /data. Adds docs/TESTING.md as the test handbook, covering each layer's proves / does-not-prove / commands / prerequisites, the browser layer and its untested-when- unreachable behaviour, the Windows WinError 193 class, and why the reviewers found what our own checks did not. AGENTS.md gains a required review-and-validation discipline built from those same defects.
JupyterLab runs browser, run detail, logs and loss surfaces, plus a toolbar submit flow that turns the open notebook into a TauGrid RayJob. The wheel ships the prebuilt labextension and enables the server extension on install. Submission resolves the current-generation ready status catalog, enforces applicability for the namespace the run lands in, and refuses a GPU profile unless the request declares that the notebook dispatches its GPU work through Ray. Status is derived once, in describe_run/explain_run, and read by the runs browser, the detail panel and the log surface, so one run cannot be explained two ways. Authorization. A profile's applicability may not be satisfied by a value the request supplies about itself. Team and lane come from the Jupyter server's operator-set environment; a request body that carries them is rejected. A namespace-scoped profile is only usable for a namespace the server can actually list. Every handler that reads or writes cluster state consults the Jupyter Authorizer, per namespace, and fails closed without one; authentication is not authorization. The boundary authenticates the server, not the individual caller, so a multi-team deployment runs one server per team - documented rather than implied. The profile's resolved queue is canonical: a request cannot spend another team's quota by naming a different LocalQueue. The submitter entrypoint is framed so KubeRay's shell cannot re-split it, which is what previously ran the notebook in the submitter instead of on the Ray head. Terminal state comes from the current lifecycle, never from an attempt counter that stays positive across a retry. The rendered workload stamps the identity labels the shared Go consumers read, checked cross-language against the real parsers. The panel cleans the staging directory it created. Enabling submission without a runtime image that can execute a notebook now fails at startup. Test and review discipline. scripts/ci/check-review-patterns.py turns the review rules into 18 checks with defect fixtures, run in CI and make test. It exists because a shape list drawn from one review round passed on a tree that still held eighteen reviewer-found defects; the checks encode the stance instead - follow a denial path to the write it guards, find the owner of the final result, trace every consumer of a shared contract, exercise the real entry path, and prove an assertion can fail. skills/taugrid-review/ is the reviewer's statement, and AGENTS.md records what a reviewer must do that the author's green run cannot. Go toolchain and golang.org/x/net move to 1.26.9 and v0.60.0 for the advisories against 1.26.7 and v0.59.0. The go directive stays at the older minimum because the image builders use golang:1.26.8.
98317c7 to
337c852
Compare
… pin check The review-pattern check required the literal string `go-version-file: .go-version`. The Go module matrix job selects one module's own go.mod and falls back to `.go-version` through a GitHub expression, which pins the toolchain but is not that literal - so the check failed the build on a job that was already correct. It now accepts a `go-version-file` whose value contains `.go-version`, including inside an expression, while still requiring the reference. `--self-test` confirms the defect fixture (a govulncheck job with no pin at all) still fails the check, so this removes a false positive without weakening detection.
A /destinations request that omitted amespace authorized only the coarse augrid resource, then discovery resolved the first visible namespace and read its profiles and LocalQueues. Resolve the namespace under the cluster-wide grant and authorize augrid:<resolved> before those reads, so a per-namespace Authorizer policy is enforced on the omitted-namespace path too.
…ot break a run A notebook saved against a local virtualenv records that environment's kernelspec name in its metadata. The executor passed no `kernel_name` to nbconvert's ExecutePreprocessor, which then honours the notebook's own kernelspec - so the runtime image was asked for a kernel it does not have and the run failed before executing a single cell. The generated runner now pins `kernel_name` to the image's standard `python3` kernel. The test asserts the argument rather than merely tolerating it: the stub records the value and fails if it is missing, and reverting the runner's `kernel_name` in a scratch copy makes the test fail, so it is a control rather than a restatement. Regenerates `core/testdata/notebook-rayjob.json`, which pins the rendered manifest including the payload digest - changing the runner changes that digest, which is exactly what the fixture exists to catch. Both Go consumers still accept the regenerated fixture: `go test ./runs/... ./status/... -run Notebook` passes.
…lspec Adds a focused regression that a notebook naming a non-default kernelspec still reaches the image's own kernel. The stub test updated alongside the runner asserts the argument; this one drives the runner path itself, so the guard does not depend on a single stub's signature. Both fail when the runner's `kernel_name` is reverted, so neither is a restatement of the implementation.
| or declaration.get("priorityClass") | ||
| or spec_priorities.get("podPriorityClassName") | ||
| ) | ||
| return Profile( |
There was a problem hiding this comment.
Please preserve the resolved profile's placement and workload priority, or reject profiles this renderer cannot represent. The shipped two-worker same-network-domain profile is accepted here, but conversion drops these fields and the final worker template has no Kueue topology annotations. That silently removes the required co-location and admission priority; nodeSelector and pod priority do not replace them. Add a resolved-profile-to-final-manifest regression test.
| }, | ||
| "node_modules/@antfu/install-pkg": { | ||
| "version": "2.0.1", | ||
| "resolved": "https://ms-feed-25.pkgs.visualstudio.com/1es-public/_packaging/npm-public/npm/registry/@antfu/install-pkg/-/install-pkg-2.0.1.tgz", |
There was a problem hiding this comment.
Please regenerate this lockfile against the public npm registry before publishing it. The resolved dependencies contain organization/project-scoped package-feed URLs, exposing internal infrastructure identifiers and binding downloads to that feed. Use an environment without internal npm configuration and verify that the regenerated URLs are public.
This branch has not been deployed
3 participants
Summary
Adds the TauGrid notebook integration as a JupyterLab 4 prebuilt extension plus a
Jupyter Server extension shipped inside the
tauPython wheel. The researcher stays inJupyterLab: submit the notebook they are already working in, then watch status, logs and a loss
curve until the run finishes.
The browser never talks to Kubernetes. The Jupyter Server extension drives the Python SDK on the
Jupyter Server identity, so the notebook needs no cluster credentials and no
taubinary.The workflow
run name, namespace, queue, profile, payload sizes, excluded cells, submission mode, retention.
Surface placement
Each capability sits on the JupyterLab surface it belongs to, rather than one mega-panel:
taugrid-runs)taugrid-detail)taugrid-logs)The panel uses the TauGrid Portal design system exactly - the same tokens, type scale, cards,
stat tiles, badges, tables, buttons, focus ring and chart treatment.
What is in it
Server (Jupyter Server extension)
_payload.py- CLI-parity embedded payload: envelope v2, immutable encoding, the 64 KiBenvironment-entry cap and 1 MiB decoded ceiling, digest verified by the
tau-payloadinitContainer.
_notebook_pkg.py- validates the saved notebook, strips outputs and execution counts beforemeasuring, drops approved launcher cells, and embeds the notebook with the runner and a digest
context manifest.
_render.py- renderer parity withcli/internal/rayjobrender: Kueue queue label,tau.azure.com/managed-by,K8sJobModefor plugin submissions so the driver output is areadable pod log, and a longer retention so a finished run can still be read.
submit.py/_notebook_submit.py- plan-first submission behindTAUGRID_SUBMISSION_ENABLEDwith an explicit confirm, bound to the reviewed manifest digest.runs.py,metrics.py- run discovery for Jobs and RayJobs with lifecycle phases andowner-UID-scoped evidence, and bounded loss metrics with explicit truncation reasons.
Frontend
Portal and keyboard operable.
step=N loss=Vobservations; a bounded window, not thecomplete history, and it says so. One observation stays a point; nothing is smoothed or invented.
Demos and packaging
examples/notebook-ray-cpu-demo.ipynb(Ray CPU tasks) andexamples/notebook-loss-curve-demo.ipynb(a deterministic 40-step loss curve).images/notebook-runtime/Dockerfile- the runtime image the notebook executor needs, builtoffline from vendored wheels; selected with
TAUGRID_RUNTIME_IMAGE.share/jupyter/labextensionsand enables the serverextension through
etc/jupyter/jupyter_server_config.d, so a clean install registers it.Whole-PR audit and re-implementation
The PR was audited end to end and re-implemented rather than patched. Fourteen findings with
file:lineevidence, the decisions, and a 50-path inventory are indocs/design/notebook-plugin.md. The corrections that matter:review and confirm is rejected instead of silently submitted.
separate policies, so they cannot drift apart again.
replacement pod cannot be read under a stale identity.
preserved; the launcher-cell detection that could discard user code was replaced.
Removed as dead weight, with rationale recorded: the branch's uncalled Go TensorBoard renderer and
Portal proxy route (unbounded reads, non-empty-string-only validation, superseded by the native
extension), the
_conformancescaffold, a stale executed example notebook, committed notebookoutputs, and the superseded ipywidgets-era screenshots.
Live verification
Driven against a local JupyterLab and a docker-desktop cluster with Kueue + KubeRay:
tools/run-cpu-ray-demo.pysubmittedexamples/notebook-loss-curve-demo.ipynbascpu-loss-demo-2on the CPU profileazure.research.cpu.small(queuejobqueue,K8sJobMode,gpusPerWorker=[0]). The submitter ran the notebook, streamed real observations(
step=0 loss=9...step=6 loss=0.086),GET /status?includeMetrics=truereturned a boundedwindow, and the driver reported RESULT SUCCEEDED WITH LOSS.
tools/run-labextension-e2e.mjsdrives Edge through five journeys - open the runs sidebar, open arun detail, render the loss curve, open logs, submit a notebook through review and confirmation.
PASS, with
loss curve points: 40, polyline: 1.submitterPodTemplate: the K8sJobMode submitter had no/scriptpayload and no/datamount, the entrypoint died oncd /data, and the RayJob stillreported success. Now wired and locked by tests.
Review fixes and live verification
Run end to end on Docker Desktop Kubernetes with the real chart, workspace and
runtime image:
destinationsresolves 16 ready profiles,previewreturns aplan,
submitcreates the RayJob, Kueue admits it, and the notebook executes inthe runtime image.
gpuExecution=ray; headnum-gpus=0with workergpu=1The live run found a real defect. KubeRay 1.6 moved the submitter Job's retry
count to
spec.submitterConfig.backoffLimit. With the field unset KubeRaydefaulted the submitter Job to 2 retries while the RayJob CRD still defaulted
spec.backoffLimitto 0, so a deterministic notebook failure was retried, theretry re-attached to the finished Ray job and exited 0, and the RayJob reported
"Job finished successfully" (observed: submitter Job
backoffLimit=2,failed=1,succeeded=1, two submitter pods). Both renderers now pinsubmitterConfig.backoffLimit: 0, and the status reader also treatsstatus.failed > 0and KubeRay's "Job submission has failed" message as failure,because KubeRay can still leave
jobStatusat SUCCEEDED in that case.Review fixes on top of the original PR, all covered by regression tests:
Profile readiness comes from the resolved status catalog. The plugin read a status field
inside each
spec.workloadProfilesentry, which the CRD never populates, so missing, stale andunready profiles were all selectable. Readiness now comes from
status.workloadProfiles.profiles(whose entries embed
WorkloadProfileinline), and a generation mismatch withholds the set.Applicability is enforced, not displayed. Namespace, team and lane are checked with
empty-means-global semantics before offering a profile and again before rendering, mirroring
core/resourceprofile. An unknown scope value does not authorize a scoped profile.A GPU profile no longer implies the notebook gets a GPU. The notebook runs in the RayJob's
control-only head, so
gpusPerWorkersizes Ray workers. A GPU profile is refused unless therequest declares
gpuExecution=ray, and the catalog reportsnotebookKernelGpu/requiresRayDispatchso the picker states it.Browser harnesses delete only what they created. Each run takes a unique workspace and session
identity and tears down exactly the ids it created; the previous path-only match and name sweep
could terminate a person's kernel or a concurrent run.
Bounded reads work on the supported dependency floors. The reader returned the raw
HTTPResponsewhenever the client had noread1. urllib3 1.26 has noread1, so on thoseinstalls discovery, submission planning and log reads received an unread response object. The
reader now decodes through either generation and fails closed when no deadline can be armed;
the
urllib3floor moves to>=1.26.20because 1.26.0 cannot be imported on Python 3.12+.docs/design/notebook-plugin.mdis updated with the matching decisions, API field, security framingand test posture.
Checks
python scripts/check-license-headers.pynpm run build:lib/build:labextensionnpm testruff check tau tests setup.pykubernetes==30.1.0,urllib3==1.26.20, noread1)The broader SDK (54) and Portal (16) suites are not green, but both failure sets are identical on
untouched
main- pre-existing Windows/environment failures, deliberately not fixed here.Deferred
(the prepared notebook, the runner, and the context manifest); there is no picker yet. The CLI
offers
--extra-script SRC:DEST, a project archive handed to Ray asworking_dir, andimage/PVC sources for anything large. Bringing that choice into the review is the obvious next
step, bounded by the payload caps.
authenticated portal series, and multi-rank aggregation.
behind
TAUGRID_SUBMISSION_ENABLEDuntil a certified image is recorded.Try it
pip install "tau[widgets]" docker build -t taugrid-notebook-runtime:local images/notebook-runtime TAUGRID_SUBMISSION_ENABLED=1 \ TAUGRID_RUNTIME_IMAGE=taugrid-notebook-runtime:local \ TAUGRID_PORTAL_URL=https://your-portal \ jupyter labDesign source, API surface, metrics contract and the audit record:
docs/design/notebook-plugin.md. Task-oriented docs:site/content/en/docs/examples/notebook-plugin.md.