fix(workbench): cap auto-detected resource profiles in the capacity scenario - #635
Merged
Merged
Conversation
…cenario The session-capacity scenario launches one session in every enabled profile it auto-detects from the New Session dialog. A deployment that advertises a set of profiles cannot necessarily run all of them at once: the CI Workbench container offers Default, Small, Medium and Large, which together request 8 CPUs and 30 GB from a 4-vCPU runner. The result was a nightly that failed on a different profile each run, reporting a capacity failure that was really the runner running out of room. Auto-detection now launches only the two smallest profiles, ordered by the allocation Workbench prints in the label itself. An explicit workbench.session_profiles list is never capped, so anyone who genuinely wants to exercise more says so. Because VIP is a verification tool, capping is disclosed rather than silent: it warns through both warnings.warn and the logger, matching oidc_login_lock, and the message names the profiles chosen, the ones skipped, and the config key that overrides the cap. Closes #631
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped to the capacity auto-detection path, includes disclosure, and is covered by targeted selftests; remaining feedback is minor.
Pull request overview
This PR adjusts the Workbench session-capacity test to avoid over-subscribing constrained environments (notably CI runners) when resource profiles are auto-detected from the UI, while keeping explicit workbench.session_profiles behavior unchanged.
Changes:
- Cap auto-detected Workbench resource profiles to the two smallest (by parsing CPU/RAM from the dropdown label) to reduce CI flakiness from host resource starvation.
- Emit a disclosure warning (via
warnings.warn+ logger) when capping occurs, including which profiles are launched vs skipped and how to override via config. - Add selftests for label parsing and capping behavior, and update
vip.toml.examplecomments to document the new behavior.
File summaries
| File | Description |
|---|---|
vip.toml.example |
Documents that auto-detected profiles are capped to the two smallest and explains how explicit config overrides the cap. |
src/vip_tests/workbench/test_session_capacity.py |
Applies the cap when profiles are auto-detected from the UI. |
src/vip_tests/workbench/conftest.py |
Introduces label parsing + capping helper (and warning/log disclosure) for auto-detected profiles. |
selftests/test_workbench_session_capacity_profiles.py |
Adds unit tests for size parsing, ordering, cap behavior, and warning contents. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Addresses two review comments. The memory figure in profile_size_key used a 1024 multiplier while being named megabytes; it is mebibytes, and the name now says so, with a note that the exact unit is irrelevant because the value is only ever a sort key. The cap warning joined profile labels with ", " but the labels embed commas of their own -- "Medium (2 CPUs, 8GB RAM)" -- so the chosen and skipped lists read as one run-on list. Each label is now quoted via a small _quoted helper, matching the !r convention used elsewhere in this package. Adds a selftest pinning the quoted form, since the whole point of the warning is that a human can read what was skipped.
Contributor
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #631
The session-capacity scenario launches one session in every enabled profile it auto-detects from the New Session dialog. A deployment that advertises a set of profiles cannot necessarily run all of them at once: the CI Workbench container offers Default, Small, Medium and Large, which together request 8 CPUs and 30 GB from a 4-vCPU runner. The result was a nightly that failed on a different profile each run — Medium on 09-08, Small on 09-04 — reporting a capacity failure that was really the runner running out of room.
Auto-detection now launches only the two smallest profiles, ordered by the allocation Workbench prints in the label itself, so no profile name is hardcoded and nothing breaks when
posit-dev/with-workbenchchanges them. An explicitworkbench.session_profileslist is never capped, so anyone who genuinely wants to exercise more says so.Because VIP is a verification tool, capping is disclosed rather than silent: it warns through both
warnings.warnand the logger, matchingoidc_login_lock, and the message names the profiles chosen, the ones skipped, and the config key that overrides the cap.Why this shape
Two alternatives were considered and rejected. Pinning
session_profilesin the workflow's generatedvip.tomlwould have hardcoded exact labels that this repository does not own, andselect_option(label=...)needs an exact match, so a label change would hard-fail the nightly. Dropping the scenario from the Docker tier would have stopped the red without fixing anything. Capping in the test also helps a real user running VIP against a modest deployment, which is the same failure mode.Caveat
This is a hypothesis, not a reproduced fix — the over-subscription cannot be reproduced locally and needs a real nightly run.
test_session_suspend_resumefails in all five of the recent nightly failures withreached terminal state 'Failed', which is consistent with resource starvation, but if it still fails after this merges then it is a separate problem and #631 says to split it out.Testing
1922 selftests pass, 18 of them new, covering label parsing, the cap itself, tie-breaking order, the non-positive-limit escape hatch, and that the warning names both the chosen and the skipped profiles.
ruff check,ruff format --checkandmypy src/vip/are clean.