[Misc] Add override for allreduce fusion thresholds #23639
Conversation
Signed-off-by: Julien Lin <[email protected]>
Signed-off-by: Julien Lin <[email protected]>
nvjullin
requested review from
zou3519,
youkaichao and
ProExpertProg
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an environment variable to override allreduce fusion thresholds, which is a good feature for benchmarking. However, the implementation lacks robust error handling for the user-provided configuration. My review includes suggestions to prevent application crashes from malformed JSON or invalid data types in the environment variable, improving the overall robustness of this new feature.
nvjullin
changed the title
Signed-off-by: Julien Lin <[email protected]>
Signed-off-by: Julien Lin <[email protected]>
github-actions
bot
added
the
ready
| # { <world size>: <max size in mb> } | ||
| # Unspecified world sizes will fallback to | ||
| # { 2: 64, 4: 1, <everything else>: 0.5 } | ||
| "VLLM_FLASHINFER_ALLREDUCE_FUSION_THRESHOLDS_MB": |
There was a problem hiding this comment.
Could we make this a CompilationConfig.PassConfig variable? It might be good to be tunable even after #22086 lands
| _FI_MAX_SIZES.update({ | ||
| int(k): int(float(v) * MiB) | ||
| for k, v in | ||
| envs.VLLM_FLASHINFER_ALLREDUCE_FUSION_THRESHOLDS_MB.items() |
There was a problem hiding this comment.
I think reading this will only run at startup but we generally want to read envs during init in case they change between different LLM instantiations. Putting it in config and reading it at pass init time would be best.
There was a problem hiding this comment.
@ProExpertProg @ilmarkov Should we just replace fi_allreduce_fusion_max_token_num with something like fi_allreduce_fusion_thresholds_mb? Why do we need to check both num_tokens and message size?
There was a problem hiding this comment.
|
@nvjullin did not realize this was on automerge, can you address comments in a follow-up please? |
Signed-off-by: Julien Lin <[email protected]> Signed-off-by: tc-mb <[email protected]>
Signed-off-by: Julien Lin <[email protected]>
Signed-off-by: Julien Lin <[email protected]> Signed-off-by: Xiao Yu <[email protected]>
Signed-off-by: Julien Lin <[email protected]> Signed-off-by: Xiao Yu <[email protected]>
Signed-off-by: Julien Lin <[email protected]>
Signed-off-by: Julien Lin <[email protected]>
Signed-off-by: Julien Lin <[email protected]>
Signed-off-by: Julien Lin <[email protected]>
Purpose
Long term plan is being discussed at #22086, but it is not easy and will take some more time.
Meanwhile, add an override so we can benchmark the effect of tuning the thresholds and unblock people.
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.