Repository navigation
fix: a gang's min_instances is 0 or batch_size and a kept-warm gang binds together - #636
Merged
Merged
Conversation
This was referenced Oct 27, 2026
k82cn
force-pushed
the
fix/gang-max-instances-and-e2e
branch
from
October 27, 2026 07:39
c9a0a69 to
0652662
Compare
k82cn
force-pushed
the
fix/gang-max-instances-and-e2e
branch
2 times, most recently
from
October 27, 2026 08:36
bdcbc41 to
c1c9f3c
Compare
k82cn
force-pushed
the
fix/gang-max-instances-and-e2e
branch
from
October 27, 2026 08:49
c1c9f3c to
1f62afa
Compare
This was referenced Oct 27, 2026
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.
TestGangScheduling::test_single_instance_sessions_are_not_held_backfails intermittently in the Python E2E jobs (seen on #621, #624,drfandpriority). The cluster state printed by #630 showed why: six idle executors left by the previous test, allBoundto the 7-instance gang at the timeout, and none for the single session.Cause
_create_gangcreates the session withmin_instances=sizeandbatch_size=size, then its task.batch_size()inscheduler/plugins/mod.rscounted the gang only while the session had a pending task, so between the two calls the session was not a gang:min_instancesmade it underused,is_satisfiedwas trivially true, and Dispatch bound idle executors to it one by one. A gang of 7 on a cluster of 6 then held all six, could not get a seventh, and was not over its demand, so Shuffle did not take them back; the single session never got an executor. A scheduler cycle (every 100 ms) has to land in the few milliseconds between the two calls, which is why it is intermittent. Reproduced deterministically in the in-process scheduler with a cycle between the two calls; the new scheduler test below is that scenario.Change
A gang (
batch_sizeabove 1) is the whole session, so its instance limits are tied to its size:create_session, andopen_sessionwith a spec, reject (INVALID_ARGUMENT) a session withbatch_sizeabove 1 unlessmin_instancesis 0 orbatch_sizeandmax_instancesis unset orbatch_size, and unless the application'smax_instancesis at leastbatch_size. A session without a gang is rejected only if itsmax_instancesis below itsmin_instances; amax_instancesof 0 without a gang stays valid (it prevents allocation, as documented).is_satisfiedtreats a gang kept warm (batch_sizeabove 1 andmin_instancesabove 0, which the validation makes equal) as a gang whether or not a task is pending, so it is formed all at once like any gang, even before its first task. The demand already includesmin_instances, so Shuffle does not reclaim its executors (is_overusedis false atbatch_sizebound).batch_size()inplugins/mod.rsis unchanged.Nothing else in Dispatch, Allocate or Shuffle changes. The e2e gang tests are unchanged: they already create gangs with
min_instances=size.Formatting
rustfmt.tomlsetssingle_line_if_else_max_width = 60(the default is 50), so a shortif/elsesuch aslet kept_warm = if ssn.batch_size > 1 { ssn.min_instances } else { 0 };stays on one line.cargo fmt --allreformatted three otherif/elseexpressions to match (common/src/storage/engine/sqlite.rs,executor_manager/src/appmgr/downloader.rs,flmctl/src/list.rs); the changes are whitespace only. A branch that is not rebased will failcargo fmt --checkon those spots until it runscargo fmt.Compatibility
Sessions with
batch_sizeabove 1 and anothermin_instancesormax_instancesare now rejected when created. In the repository, onlyexamples/rl/search-r1's trainer sets a gang (batch_size=TRAINERS, no warmup, autoscaling:min_instances0 andmax_instancesunset), which stays valid. From the Python SDK,warmupmust be 0 orbatch_size, andautoscale=False(which fixes the maximum atwarmup) needswarmup=batch_size. RFE591,docs/api/types.mdandsdk/python/docs/API.mdsay so.Tests
controller/mod.rs:gang_min_and_max_instances_are_its_size_or_not_set: each rejected combination throughcreate_sessionandopen_session, and the accepted ones (gang alone, gang kept warm, exact limits, no gang with its own limits, a limit of 0 without a gang).scheduler/mod.rs:test_gang_kept_warm_binds_all_at_once_even_without_a_task, withpriority+dasanddrf+das: a gang of 7 withmin_instances7 on 6 slots binds nothing with or without a task, binds all 7 on 7 slots with or without a task, and one withmin_instances0 and no task binds nothing. It fails withmain'sbatch_size().plugins/minmax.rs: a gang kept warm is satisfied only when whole before its first task, and is never over its demand; the demand tests covermin_instancesequal tobatch_size.cargo test -p flame-session-manager(299 passed with--test-threads=6; at the default parallelism some tests hit the per-process file limit on my machine), clippy-D warnings, rustfmt.This replaces the earlier contents of this pull request (a different limit rule and a rework of the e2e gang tests) and supersedes #637.