fix(kvbm): remove the transfer batch size knob that nothing reads - #14011
fix(kvbm): remove the transfer batch size knob that nothing reads#14011ayaangazali wants to merge 1 commit into
Conversation
DYN_KVBM_TRANSFER_BATCH_SIZE is declared in environment_names.rs and documented in the KVBM configuration reference as "Maximum number of blocks per transfer batch", but no code anywhere reads it. The knob that does work is DYN_KVBM_MAX_TRANSFER_BATCH_SIZE, added by ai-dynamo#7527 and read through offload.rs::max_transfer_batch_size. The two sit next to each other in the reference with near-identical descriptions, so the dead one is the more natural name to reach for and setting it does nothing and reports nothing. Remove the constant and its documentation entry rather than wire it up, since the working knob already covers the behaviour. Also correct the defaults on the two neighbouring entries: the reference lists both as "unset" when offload.rs defaults them to 16 and 4. Signed-off-by: ayaangazali <ayaangazali.work@gmail.com>
|
👋 Hi ayaangazali! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
Bot
added
external-contribution
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe KVBM transfer-batch documentation now defines a maximum batch size of 16. The runtime environment declarations remove the previous transfer configuration module and environment constant. ChangesKVBM transfer-batch configuration
Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: ⚪ Minimal · up to This localized change removes an unused KVBM configuration entry and aligns documented defaults with the existing behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the change, rationale, affected identifiers, corrected defaults, and validation results. It does not follow the required template because it omits the required Related Issues section and does not identify where the reviewer should start. Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.)
Comment |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
Summary
DYN_KVBM_TRANSFER_BATCH_SIZEis declared inenvironment_names.rsand documented in the KVBM configuration reference as "Maximum number of blocks per transfer batch". Nothing reads it. The only two references in the repository are the constant and the docs entry:The knob that actually works is
DYN_KVBM_MAX_TRANSFER_BATCH_SIZE, added by #7527 and read throughoffload.rs::max_transfer_batch_size.This matters because of where the dead name sits. The reference lists the two adjacently with near-identical descriptions, "Maximum number of blocks per transfer batch" for the dead one and "Upper bound on the transfer batch size" for the live one, so the dead name is arguably the more natural one to reach for. Setting it changes nothing and warns about nothing.
Removed rather than wired up: the live knob already covers the behaviour, and inventing semantics for a second one would add a tuning surface nobody asked for.
While in the same three entries, the defaults are also wrong.
offload.rsdefaultsDYN_KVBM_MAX_TRANSFER_BATCH_SIZEto 16 andDYN_KVBM_MAX_CONCURRENT_TRANSFERSto 4, and the reference lists both asunset.Validation
Local, on macOS.
cargo build -p dynamo-runtimeclean.cargo test -p dynamo-runtime --lib config::environment_namesis 2 passed, 0 failed, coveringtest_no_duplicate_env_var_namesandtest_naming_conventions.cargo fmt --all -- --checkclean,cargo clippy -p dynamo-runtime --libclean.pre-commit run --files <the two files>passes every hook that applies, including the Fern asset-path check.pytest-marker-reportfails identically on an untouchedREADME.mdhere because vLLM and TensorRT-LLM are not installed.The removed module was unreferenced:
grep -rn "kvbm::transfer\|transfer::DYN_KVBM" --include='*.rs' libreturns nothing, which is also what the clean build confirms.I did not add a test. The removal is covered by the build, and the two corrected defaults are documentation values with the constants
DEFAULT_MAX_TRANSFER_BATCH_SIZE = 16andDEFAULT_MAX_CONCURRENT_TRANSFERS = 4as their source of truth inoffload.rs.Summary by CodeRabbit
DYN_KVBM_MAX_TRANSFER_BATCH_SIZE.