Repository navigation
Add job template: contract wrapper, sample tool, smoke test, ghcr build - #3
Conversation
Implements the mmli-backend job contract (job.json in, results.json plus artifacts out, non-zero exit on failure) with a runnable sample tool, schema-validated smoke test, licensed-content checklist, and a GitHub Actions build that pushes immutable sha-/semver tags to ghcr. Closes #1
matthewberry
left a comment
There was a problem hiding this comment.
Built the image and ran scripts/smoke-test.sh locally: passes (OK: output satisfies the contract). I checked the README's contract claims against mmli-backend origin/main (0f0e797). Two findings and a few notes are inline.
Checked and cleared
- Omitting
commandis safe:job.tpl.yamladds thebash -c … touch finishedwrapper only whencommandis set, and a grep oforigin/main:appfound no readers of thefinished/errormarker files outside the template. - Failure detection:
kubejob_service.py:309-316treats a Failed condition orstatus.failed > 0as terminal, so a non-zero exit does fail the job. /code/app/cfg:config.yamlandsecret.yamlare mounted there in the job container (job.tpl.yaml:229-234).- ghcr name casing:
docker/metadata-actionlowercases image names (src/meta.ts:655), soiBioFoundryrepos get valid names. - Artifact names
./..: the output schema's pattern allows them, butvalidate.pyreports them as missing (is_file()is false), so the smoke test catches them.
Not checked
- The workflow itself: run 39297799026 was still on "Free runner disk space" when I looked. The push path runs only after merge.
- The negative cases in the PR description. I re-ran only the happy path.
|
|
||
| # Same env vars the backend sets; run as the current user so the output is removable | ||
| docker run --rm \ | ||
| --user "$(id -u):$(id -g)" \ |
There was a problem hiding this comment.
The smoke test runs as a different user than the cluster. On GitHub runners this is uid 1001, which has no /etc/passwd entry in python:3.12-slim, so HOME=/. The backend runs jobs as root (every securityContext in config.yaml sets runAsUser: 0) and mounts the weight caches under /root/.cache/... (torch hub, huggingface hub, SimpleFold).
Reproduced:
$ docker run --rm --user 1001:1001 python:3.12-slim python -c "import pathlib; (pathlib.Path.home()/'.cache').mkdir(parents=True, exist_ok=True)"
PermissionError: [Errno 13] Permission denied: '/.cache'
So a tool that downloads weights to the HF/torch default location works on the cluster but fails in CI. The sample tool doesn't hit this because it uses only the standard library. Fix: add -e HOME=/tmp, or run as root like the backend and clean up $work with a docker run … rm -rf.
There was a problem hiding this comment.
Reproduced (PermissionError on /.cache as uid 1001; /root as root). Fixed in 5b9df65: the smoke test now runs the container as root like the backend and removes its root-owned output from inside a container. Verified /root/.cache/huggingface is creatable in the smoke environment.
| # && git -C /opt/method checkout "${METHOD_REF}" \ | ||
| # && rm -rf /var/lib/apt/lists/* | ||
| # Do not copy model weights into the image; download them at runtime into the cache | ||
| # path described in the README. |
There was a problem hiding this comment.
The cache path isn't defined anywhere. This comment points to "the cache path described in the README", but the README ("Things that will bite you") only says "a cache directory backed by the shared volume… as SimpleFold does" and never names a path or env var. That leaves #1's first acceptance criterion ("fetched at runtime into a cache path") only partly met. The template should state the convention, e.g. download to ~/.cache/<lib>, which the backend mounts at /root/.cache/.... Better still would be an env var like JOB_CACHE_DIR that the backend sets. Whichever path you pick also needs to be writable in the smoke test (see the smoke-test.sh comment).
There was a problem hiding this comment.
Fixed in 5b9df65. Convention: libraries download to their default cache under /root/.cache; the backend's values mount uws/jobs/<tool>/.cache at /root/.cache. Stated in the Dockerfile comment and the README. I chose the path convention over a JOB_CACHE_DIR env var because it needs no backend change and matches how SimpleFold already caches.
There was a problem hiding this comment.
Correction to my reply above: SimpleFold doesn't mount all of /root/.cache. It mounts its subpath at /root/.cache/torch/hub, and UniKP and ReactionMiner mount theirs at /root/.cache/huggingface/hub. The README's convention of mounting all of /root/.cache is new, not a copy of SimpleFold's setup. It still works with the existing /root/.cache/token secret mount on top. (From review round 4.)
| | Variable | Meaning | | ||
| |---|---| | ||
| | `JOB_INPUT_DIR` | Contains `job.json` (the job input, already validated against the tool's input schema) and any files uploaded with the job | | ||
| | `JOB_OUTPUT_DIR` | Everything written here is uploaded and kept as the job's output | |
There was a problem hiding this comment.
Note: this holds only on success. In job.tpl.yaml the job container is an init container and post-job is the regular container, so after a non-zero exit the pod fails and postjob.py never runs. Nothing is uploaded at all, logs included. That's fine, but worth saying here. It also means the atomic results.json rename guards against a case the cluster can't reach (harmless to keep).
There was a problem hiding this comment.
Agreed. In 5b9df65 the README now says a failed job uploads nothing, and I dropped the tmp-file rename, since it guarded a case the cluster can't reach.
| ## The contract | ||
|
|
||
| Through the backend's generic job path | ||
| ([mmli-backend#144](https://github.com/moleculemaker/mmli-backend/issues/144)), the |
There was a problem hiding this comment.
Note: mmli-backend#144 is still open (and blocked on this issue), so "the image is launched with no command… with these environment variables" describes the planned path, not the current one. A "planned" note until #144 lands would avoid confusion.
There was a problem hiding this comment.
Fixed in 5b9df65: the README marks the generic path as planned in mmli-backend#144 and says per-tool branches are still needed until it lands.
|
|
||
| # Written last and renamed into place, so results.json exists only if the tool finished | ||
| tmp = output_dir / "results.json.tmp" | ||
| tmp.write_text(json.dumps(results, indent=2) + "\n") |
There was a problem hiding this comment.
Note: json.dumps defaults to allow_nan=True, so a tool that returns a NaN score writes NaN, which isn't valid JSON. validate.py reads the file back with Python's json.loads, which accepts NaN, and the output schema allows it, so CI passes while the browser's JSON.parse fails. allow_nan=False would make the job fail loudly instead. The sample tool can't produce NaN; this would affect tools built from the template.
There was a problem hiding this comment.
Reproduced (json.dumps(float('nan')) gives NaN). Fixed in 5b9df65 with allow_nan=False. Verified that a tool returning NaN now fails the job (ValueError, exit 1) and leaves no results.json. The README tells tools to use null for missing values.
| with: | ||
| python-version: '3.12' | ||
|
|
||
| - run: pip install jsonschema |
There was a problem hiding this comment.
Nit: jsonschema is unpinned here, although requirements.txt asks tools to pin everything.
There was a problem hiding this comment.
Pinned to jsonschema==4.26.0 in 5b9df65.
|
|
||
| - name: Push | ||
| if: github.event_name == 'push' | ||
| uses: docker/build-push-action@v6 |
There was a problem hiding this comment.
Note: this builds the image again rather than pushing the job:test image the smoke test ran. Within one job the buildx cache makes them identical in practice, so this isn't a defect, just an observation.
There was a problem hiding this comment.
Leaving this as is. Within one job the buildx cache makes the two builds identical.
- smoke-test.sh runs the container as root like the backend, so default library caches under /root/.cache are writable; cleans up from inside a container since the output is root-owned - Cache convention: libraries' default /root/.cache, with the backend mounting uws/jobs/<tool>/.cache there - entrypoint writes results.json with allow_nan=False; drop the tmp-file rename, since a failed job uploads nothing anyway - README: generic path is planned (mmli-backend#144); failed jobs have no output; pin jsonschema in CI
matthewberry
left a comment
There was a problem hiding this comment.
Re-review of 5b9df65. Every earlier thread is addressed. I re-ran the smoke test locally on 5b9df65 and it passes, and CI run 39299293847 is green. I confirmed the NaN change myself: forcing a NaN into the sample tool makes the entrypoint raise ValueError: Out of range float values are not JSON compliant: nan, exit 1, and leave no results.json. Dropping the tmp-file rename is fine, because json.dumps raises before the file is opened and a failed job uploads nothing.
One new finding, inline: the smoke test now runs as the image's default user rather than explicitly as root, so the earlier mismatch comes back in reverse for base images that set a non-root USER.
Not checked: the push-to-ghcr path, which runs only after merge.
| cp "$root/examples/job.json" "$work/in/job.json" | ||
|
|
||
| # Same env vars and user (root) as the backend, so default cache paths under /root/.cache work | ||
| docker run --rm \ |
There was a problem hiding this comment.
This runs as the image's default user, not necessarily root. Dropping --user gives root only when the image's USER is root. That holds for python:3.12-slim and the CUDA images, but not for micromamba, which the backend already uses (see the somn entry in config.yaml, "run as mambauser => 57439"). Reproduced:
$ docker run --rm --entrypoint id mambaorg/micromamba:1.5.10 -u
57439
$ docker run --rm --entrypoint sh mambaorg/micromamba:1.5.10 -c 'echo HOME=$HOME; mkdir -p /root/.cache'
HOME=/home/mambauser
mkdir: cannot create directory '/root': Permission denied
In CI, a tool on that base would cache under /home/mambauser/.cache. On the cluster, with runAsUser: 0, it would cache under /root/.cache, which is where the README tells the backend to mount the volume. CI passes either way, so the mismatch stays hidden. Suggest --user 0:0 here and on the cleanup docker run in the trap. That makes the comment on line 15 true for every base image. Separately, the README's "launched as root" depends on the tool's backend values setting securityContext.runAsUser: 0 for such images; it may be worth saying that explicitly.
There was a problem hiding this comment.
Reproduced: mambaorg/micromamba:1.5.10 defaults to uid 57439, and the backend uses the image's USER unless the tool's values set securityContext (only SOMN does, with runAsUser: 0/runAsGroup: 0). Fixed in b7f019e: --user 0:0 on both docker runs, and a README bullet requiring runAsUser: 0/runAsGroup: 0 in the tool's values for non-root base images. Verified with a micromamba-based build of the template: the default user is 57439, but the smoke test passes running as root with HOME=/root.
There was a problem hiding this comment.
Correction to my reply above: SOMN isn't the only tool that sets securityContext. In mmli-backend origin/main, crispr-copies and mutagenesis also set runAsUser: 0, but only in app/cfg/config.yaml (the fallback baked into the image). chart/values.yaml, which is deployed, has it for SOMN only. So "as SOMN does" stays the right example to point at. That difference between config.yaml and the deployed values is the config-duplication drift we decided to leave alone; I've noted it for the new-tool checklist.
Some base images (e.g. micromamba, uid 57439) default to a non-root USER, so dropping --user ran the test as that user while the cluster runs as root, hiding cache-path mismatches. The backend runs the image's default USER unless the tool's values set securityContext, so the README now requires runAsUser/runAsGroup 0 for non-root base images.
matthewberry
left a comment
There was a problem hiding this comment.
Re-review of b7f019e: the root-user finding is resolved and I have nothing new to raise.
--user 0:0is now on bothdocker runcalls. The smoke test passes locally on b7f019e.- The README's new "Jobs must run as root" bullet is accurate against mmli-backend
origin/main(0f0e797).kubejob_service.py:849readssecurityContextonly from the tool's ownkubernetes_jobsentry, with no fallback todefaults, so a tool without one runs as the image'sUSER.
One small correction to the thread reply, which doesn't affect the PR: SOMN isn't the only tool that sets securityContext. crispr-copies and mutagenesis also set runAsUser: 0/runAsGroup: 0. defaults sets it too, but kubejob_service.py never reads it. "as SOMN does" in the README is still correct.
Not checked: CI run 39300164779 was still pending when I posted, and the push-to-ghcr path runs only after merge.
validate.py silently skipped listed artifacts with no file in examples/expected/, contrary to its docstring. Also pin jsonschema in the README's local instructions and fix a 'above' that meant 'below'.
matthewberry
left a comment
There was a problem hiding this comment.
Round 4, reviewing b7f019e. Everything from the earlier rounds is still resolved, and CI run 39300164779 is green. I found two new things, which are inline, plus the points below.
PR body is out of date. It says the entrypoint writes results.json "(atomic rename)", but 5b9df65 dropped the rename, and entrypoint.py:25 now writes the file directly.
Nits
README.md:84says "The cache convention above", but the convention is in the next bullet, below it.README.md:62haspip install jsonschemawith no version, while CI pinsjsonschema==4.26.0.packages: writealso applies topull_requestruns. This is harmless as written, because.dockerignoreexcludes.gitand only people with push access can open PRs from branches in this repo. Moving the push steps into a separate job would narrow it. Optional.
Checked and cleared (mmli-backend origin/main 0f0e797)
- These exist:
app/schemas/<tool>.input.json, thelicensefield intools.yaml(whichLICENSED_CONTENT.mdreferences), and theuws/jobs/<tool>/.cachesubpath pattern inconfig.yaml. <tool>.output.jsondoesn't exist yet. The README covers it inside the section it marks as planned in mmli-backend#144, and #144's acceptance criteria add it, so the README is consistent.- A correction to a thread reply, which doesn't affect the PR: SimpleFold doesn't mount at
/root/.cache. It mounts its subpath at/root/.cache/torch/hub, and UniKP and ReactionMiner mount theirs at/root/.cache/huggingface/hub. Mounting all of/root/.cache, as the README recommends, still works, and the/root/.cache/tokensecret mount can still sit on top of it.
Not checked
- The output schema's claim that "The frontend's result types are generated from this schema." I didn't look for the generator.
- The push-to-ghcr path, which runs only after merge.
| }, | ||
| "sequence": { | ||
| "type": "string", | ||
| "pattern": "^[ACDEFGHIKLMNPQRSTVWYacdefghiklmnpqrstvwy]+$", |
There was a problem hiding this comment.
A trailing newline gets past this pattern and crashes the tool. In Python regex, $ also matches just before a final \n, and the backend validates with Python jsonschema (app/routers/v1/jobs.py:121 on mmli-backend origin/main 0f0e797). Reproduced with jsonschema==4.26.0:
{"sequences":[{"id":"a","sequence":"ACD\n"}]} -> no validation errors
{"sequences":[{"id":"a\n","sequence":"ACD"}]} -> no validation errors
Running the entrypoint on the first input gives KeyError: '\n' at wrapper/tool.py:25. The job fails when the request should have been rejected with a 422. Severity is low: the browser's regex doesn't match there, so only someone calling the API directly can hit this. But tools built from the template will copy ^…$ patterns. Two ways to fix it that work in both Python and the browser: add "not": {"pattern": "\n"}, or have the tool reject unknown residues with a clear error.
There was a problem hiding this comment.
Reproduced: Python re $ accepts "a\n", while JS RegExp rejects it. Fixing it in the backend validator instead of per schema, since _validate_against_schema checks all /v1 input in one place and clean.input.json has the same problem: mmli-backend#147. Template schemas unchanged.
| errors.append("results.json differs from examples/expected/results.json") | ||
| for name in artifacts: | ||
| expected = expected_dir / name | ||
| if expected.is_file() and (output_dir / name).is_file() \ |
There was a problem hiding this comment.
Artifacts with no expected file are skipped silently. The docstring says that results.json and each artifact must match examples/expected/ exactly. But expected.is_file() here means that a listed artifact with no file in examples/expected/ is never compared. A tool that adds a second artifact and forgets its expected copy still passes. Either report the missing expected file as an error, or reword the docstring and the README.
There was a problem hiding this comment.
Fixed in 9826fdf. A listed artifact with no file in examples/expected/ is now reported as examples/expected/<name> is missing (verified).
matthewberry
left a comment
There was a problem hiding this comment.
Re-review of 9826fdf: every round-4 point is resolved or settled, and I have nothing new to raise.
validate.py: a listed artifact with no expected file is now an error. I ran it against hand-built output directories. The sample output still passes. An extra listed artifact with no expected copy fails withexamples/expected/extra.txt is missing(exit 1). A listed artifact that is missing from the output reports only the existing "listed in results.json but missing" error, not a second one.- Trailing newline: I agree with fixing this once in the backend validator instead of in each schema. I checked the claims in mmli-backend#147. In Node,
RegExp('^[A-Za-z0-9][A-Za-z0-9._ -]*$', 'u')rejects"a\n"and accepts"a".clean.input.jsononorigin/mainhas the two$-anchored patterns the issue names (lines 32 and 37). The suggested(?![\s\S])rewrite rejects"a\n"and accepts"a"in Pythonre. Until #147 lands, the sample tool still fails on that input instead of returning a 422. That's acceptable for a template. - PR body: the atomic-rename claim is gone. The body now describes
allow_nan=False, the root user, and the expected-file requirement, all of which match the code. - README nits: both are fixed.
packages: writeis left as is, which is fine.
Not checked: CI run 39301399678 was still pending when I posted. The push-to-ghcr path runs only after merge.
Closes #1.
What
wrapper/entrypoint.py: the mmli-backend job contract. Reads$JOB_INPUT_DIR/job.json, callstool.run(), writes$JOB_OUTPUT_DIR/results.jsonwithallow_nan=False, and any exception exits non-zero (a failed job uploads nothing).wrapper/tool.py: a sample tool (protein length + average molecular weight) with a CSV artifact, so the template runs end to end.schemas/: example input/output schemas in the draft 2020-12 style used bymmli-backend/app/schemas. Defines theartifactsconvention (name,media_type,description) that mmli-backend#144's generic results path will serve.scripts/smoke-test.sh+scripts/validate.py: runs the image as root with the backend's env vars, then checks the example input against the input schema, the output against the output schema, that the listed artifacts exist, and an exact match againstexamples/expected/(every listed artifact needs an expected file)..github/workflows/build.yml: builds and smoke-tests on PRs; on pushes to main, pushessha-<short>; onvX.Y.Ztags, pushesX.Y.Z.latest=false. Frees runner disk space first.LICENSED_CONTENT.mdandREADME.md.Verification
Done locally with Docker (arm64):
results.jsonleft behind); listed artifact missing (exit 1); output schema violation (exit 1).Not verified: the workflow itself. It runs for the first time on this PR. The push-to-ghcr steps run only after merge.
Follow-ups