Skip to content

fix(web): drop stale batch job resource options - #469

Merged
cswaney merged 3 commits into
mainfrom
worktree-fix-batch-job-resource-options
Aug 18, 2026
Merged

fix(web): drop stale batch job resource options#469
cswaney merged 3 commits into
mainfrom
worktree-fix-batch-job-resource-options

Conversation

@cswaney

@cswaney cswaney commented Aug 16, 2026

Copy link
Copy Markdown
Member

Summary

  • TigerFlow now runs as a single Slurm allocation (no auto-scaling), so remove the "Max Workers" input from the Compute step and stop sending max_workers in the job request (backend still defaults it to 1).
  • Rename "Worker Timeout" → "Time Limit" so the label matches what it actually sets (the Slurm job time limit via resources.time).
  • Update the job details panel: drop the "Max Workers" row, rename "Walltime (per allocation)" → "Time Limit", remove the stale PropType.

Test plan

  • npm run lint (no new warnings)
  • npm test (510/510 pass)
  • Manually launch a batch job from the UI; confirm Compute step shows Tier + Advanced Options only, and job details panel shows "Time Limit" instead of "Max Workers"/"Walltime"

TigerFlow now runs as a single Slurm allocation (no auto-scaling), so the
"Max Workers" input on the Compute step no longer applies and the "Worker
Timeout" label misdescribes what it sets. Remove Max Workers, rename
Worker Timeout to Time Limit (which maps to the Slurm job time), and
update the job details panel to match. The backend max_workers field is
left in place for now.
@claude

claude Bot commented Aug 16, 2026

Copy link
Copy Markdown

Review

Small, well-scoped cleanup — removes the now-meaningless "Max Workers" control and renames "Worker Timeout" → "Time Limit" to match what it actually sets. Verified the backend (BatchJobRequest.max_workers: int = 1 in lib/src/blackfish/server/asgi.py) still defaults the field server-side, so dropping it from the frontend request is safe, and confirmed there are no other references to job.max_workers left dangling in web/src.

Correctness

  • Looks correct. JobDetailsPanel.jsx's resource section condition correctly narrows from (job.resources || job.max_workers) to job.resources now that max_workers is never read, and the PropTypes.number for max_workers was cleaned up too.

Minor: naming now diverges from the UI label

The label was renamed "Worker Timeout" → "Time Limit", but the underlying identifiers in NewJobModal.jsx still say "worker timeout" throughout:

  • state var workerTimeout / setWorkerTimeout
  • validateWorkerTimeout, WORKER_TIMEOUT_MAX_MINUTES
  • advancedErrors.workerTimeout
  • buildJobResources's { account, workerTimeout } param
  • the input's htmlFor="worker-timeout" / id="worker-timeout"

Not a bug, but it's a bit of a trap for the next person who greps for "time limit" and finds nothing, or who reads workerTimeout and assumes it maps to a per-worker concept that no longer exists. Might be worth a follow-up rename (e.g. timeLimit/TIME_LIMIT_MAX_MINUTES) while this is fresh, especially since the PR is already touching this exact logic — low cost to do now, higher cost once more code references these names.

Test coverage

NewJobModal.test.jsx only exercises the pure helper functions (buildJobResources, isParamVisible, TASKS) — there's no rendered-component test asserting the "Max Workers" field is gone or that "Time Limit" is the label shown, so a future regression here (e.g. someone re-adding the field, or a typo in the label) wouldn't be caught by npm test. The PR body already flags manual verification as unchecked in the test plan — matches what I see in the test file. Given this is a simple UI-only change with npm test passing 510/510, this is a nice-to-have rather than a blocker.

Nit

The dropped code comment in JobDetailsPanel.jsx (/* Per-allocation walltime; batch jobs resubmit across allocations, so this is not the total job runtime. */) was explaining a real subtlety — that the Slurm time value is per-allocation, not the batch job's total wall-clock runtime. That nuance is still true after the rename (arguably still "Time Limit" without qualification, since jobs can resubmit across allocations, could read as the total job runtime to a user). Worth double-checking whether that clarification should carry forward into the new label's help text or a UI tooltip rather than being dropped entirely.

Overall: solid, low-risk cleanup that does what it says. The naming-consistency point is the only thing I'd consider addressing before merge; everything else is optional polish.

@cswaney
cswaney merged commit ed33cfb into main Aug 18, 2026
3 checks passed
@cswaney
cswaney deleted the worktree-fix-batch-job-resource-options branch August 18, 2026 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant