Adopt upstream sizing logic from ComputeManager - #26
Merged
Merged
Conversation
Drops the inline jobs_to_create math from K8SManager.create_compute_services in favor of the upstream _compute_jobs_to_create method on alchemiscale.compute.manager.ComputeManager (added in OpenFreeEnergy/alchemiscale#502). Changes ------- * K8SManager.create_compute_services signature is now (data, target) -> int. The local sizing block (min(num_tasks, job_creation_rate, max - len(server_job_names)) // claim_limit, with floor-to-1) is removed; the cycle now passes target as a kwarg. * K8SManagerSettings.job_creation_rate is deprecated. The upstream ComputeManagerSettings now provides max_submit_per_cycle for the same purpose. A model_validator(mode="before") accepts the old name on input, emits a DeprecationWarning, and migrates the value to max_submit_per_cycle. Configs that set both names use the new name and warn that the old one is ignored. Tests ----- * New tests/test_settings.py covers the deprecated-alias migration: default-inherited, explicit max_submit_per_cycle, old-name warns and migrates, both-set warns and prefers new name, old name is no longer a real field. Depends on ---------- * OpenFreeEnergy/alchemiscale#502 (must merge first; this PR will not pass tests until the upstream max_submit_per_cycle field is available). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dotsdl
marked this pull request as ready for review
June 8, 2026 21:06
Sets up the conda env, overlays alchemiscale from git main (until #502 is on a conda-forge release), installs the package, and runs the test suite. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Use a dedicated devtools/conda-envs/test.yml listing alchemiscale's runtime deps explicitly (the conda-forge alchemiscale-compute build solved in CI lacked diskcache), and ignore the unimplemented test_compute_manager stub. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the hand-listed deps with the full OpenFreeEnergy/alchemiscale devtools/conda-envs/test.yml (main), plus python-kubernetes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dotsdl
commented
Jun 9, 2026
dotsdl
left a comment
dotsdl
left a comment
Member
Author
There was a problem hiding this comment.
I'm happy with this! Merging!
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
1 participant
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.
Draft until OpenFreeEnergy/alchemiscale#502 merges. This PR depends on the new
_compute_jobs_to_createmethod andmax_submit_per_cyclefield added there.Summary
Drops the inline sizing math from
K8SManager.create_compute_servicesin favor of the upstream_compute_jobs_to_createmethod onalchemiscale.compute.manager.ComputeManager. Same formula (min(num_tasks, max_submit_per_cycle, remaining_capacity) // claim_limitwith floor-to-1), one source of truth.Changes
K8SManager.create_compute_servicessignature:(data) -> int→(data, target) -> int. The local block computingjobs_to_createis removed; the cycle now passestargetas a kwarg. The backend-specificjobs_pending()gate stays (and becomes the early-return guard, simplifying the control flow).K8SManagerSettings.job_creation_rateis deprecated. The upstreamComputeManagerSettingsnow providesmax_submit_per_cyclefor the same purpose. Amodel_validator(mode="before")accepts the old name on input, emits aDeprecationWarning, and migrates the value tomax_submit_per_cycle. Configs that set both names use the new one and warn that the old one is ignored.This means existing K8s deployments keep working with their current YAML configs — they just see a warning until they rename the field.
Tests
New
tests/test_settings.py:test_max_submit_per_cycle_default_inherited— without specifying anything, we get the upstream default of 1test_max_submit_per_cycle_explicit— explicit override workstest_job_creation_rate_migrated_with_warning— old configs continue to work but emitDeprecationWarningtest_job_creation_rate_ignored_when_new_name_also_set— when both are set, new name winstest_job_creation_rate_no_longer_a_field—model_fieldsno longer contains the deprecated name5/5 pass against the alchemiscale#502 branch. Note: existing
tests/test_compute_manager.pywas a stub (raise NotImplementedError) before this PR and remains so — out of scope here.Test plan
pytest pkg/src/alchemiscale_k8s/tests/test_settings.py— 5 tests passjob_creation_ratestill load (with a warning)🤖 Generated with Claude Code