[MLX] Size request capacity by attention DP - #32115
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
yeahdongcn
left a comment
There was a problem hiding this comment.
cc @noob-se7en (he is working on a redesign of model_runner_stub to avoid this kind of mismatch with the real model runner).
|
Thanks, updated in Changes:
Moved the local checks into the PR summary to keep this thread focused on the review feedback. |
| aux_state_size = self.server_args.max_mamba_cache_size | ||
| if aux_state_size is None: | ||
| return None | ||
| return aux_state_size // self.ps.attn_dp_size |
There was a problem hiding this comment.
Nit: please preserve the global cap for diagnostics, or convert the recommendation back to global CLI units.
The division here is correct for per-owner allocation, but _resolve_max_running_requests() later interpolates this returned value as max_mamba_cache_size and tells the user to increase --max-mamba-cache-size to at least ratio. That flag is global, while both values in the message are now per-owner.
For attn_dp_size=4 and radix ratio 4:
global cap 4 -> error reports local cap 1 and says increase the flag to at least 4
global cap 15 -> error reports local cap 3 and still says at least 4
global cap 16 -> succeeds with local cap 4 and one request per owner
Following the current advice therefore repeats the same startup failure. Please retain/report the original global value and recommend at least ratio * self.ps.attn_dp_size (16 in this case), or explicitly label the reported cap as per-owner.
There was a problem hiding this comment.
@wirybeaver Could you check if you still want to address this or just drop it? I'm fine with both (will run ci after you reply). Thanks!
There was a problem hiding this comment.
Addressed in ebb4e9cb6. I kept the allocation path per attention-DP worker, but changed the startup diagnostic back to global CLI units: it now reports max_mamba_cache_size=<global>, includes the derived per-worker cap, and recommends ratio * ps.attn_dp_size for --max-mamba-cache-size (16 for the attn-DP=4, ratio=4 case). Added test_attention_dp_auxiliary_error_reports_global_cli_units for the 15 -> recommend 16 regression.
Checked with ~/uvenv: focused MLX unit test is 4 passed; ruff, black, isort, registered-test check, py_compile all pass.
|
/tag-and-rerun-ci |
test/registered/unit/README.md requires registered unit tests to inherit CustomTestCase (from sglang.test.test_utils) rather than plain unittest.TestCase, a rule yeahdongcn has enforced before (sgl-project#32115). Both files predate the relocation and were on bare unittest.TestCase; swap is mechanical since CustomTestCase only adds a safe setUpClass/tearDownClass wrapper. test_fused_swiglu.py uses pytest-native monkeypatch/caplog fixtures throughout and is left as is; converting it away from those would be a real rewrite, not a mechanical fix, so it is reported instead of changed. Full MLX suite re-run after this change: 220 passed, 4 skipped, 0 failed.
Co-authored-by: R0CKSTAR <yeahdongcn@gmail.com>
Co-authored-by: R0CKSTAR <yeahdongcn@gmail.com>
Summary
ps.attn_dp_size, matching the canonical KV-cache resolver.--max-mamba-cache-size.Motivation
This is a prerequisite for #32101 and the Gemma 4 Apple Silicon MVP in #32102.
The MLX stub previously divided
max_running_requestsby the systemdp_size. That is correct when DP attention is enabled becauseattn_dp_size == dp_size, but it underallocates pure-DP replicas: with DP attention disabled,attn_dp_size == 1and every independent replica may need the full configured request limit.For
max_running_requests=8anddp_size=4:attn_dp_size=1, so each replica needs 8 slots;attn_dp_size=4, so each KV-cache owner needs 2 slots.This follows
KVCacheConfigurator.resolve_max_num_reqs(), which partitions the limit byps.attn_dp_size. For hybrid/linear-attention models, explicitmax_mamba_cache_sizeis also sharded per attention-DP owner before sizing the MLX auxiliary-state pool.Local Checks
Run from
~/sglang-attn-dp-request-capacityonebb4e9cb6:PYTHONPATH=python ~/uvenv/bin/python -m pytest test/registered/unit/hardware_backend/mlx/test_attn_dp_request_capacity.py -q4 passeduvx ruff@0.15.1 check python/sglang/srt/hardware_backend/mlx/model_runner_stub.py test/registered/unit/hardware_backend/mlx/test_attn_dp_request_capacity.pyuvx black@26.1.0 --check python/sglang/srt/hardware_backend/mlx/model_runner_stub.py test/registered/unit/hardware_backend/mlx/test_attn_dp_request_capacity.pyuvx isort@7.0.0 --check-only python/sglang/srt/hardware_backend/mlx/model_runner_stub.py test/registered/unit/hardware_backend/mlx/test_attn_dp_request_capacity.pyPYTHONPATH=python python3 scripts/ci/check_registered_tests.pypython3 -m py_compile python/sglang/srt/hardware_backend/mlx/model_runner_stub.py test/registered/unit/hardware_backend/mlx/test_attn_dp_request_capacity.pyThis PR contains no Gemma 4 model-specific behavior; #32102 can rebase on it after merge.
CI States
Latest PR Test (Base): ❌ Run #30464730041
Latest PR Test (Extra): ❌ Run #30464721566