Split communication logic from computation logic into orchestrator #3118
+591
−460
Conversation
This file contains 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
…ager # Conflicts: # python/sglang/srt/openai_api/adapter.py
# Conflicts: # python/sglang/srt/managers/detokenizer_manager.py
# Conflicts: # python/sglang/srt/entrypoints/engine.py # python/sglang/srt/managers/generation_manager.py # python/sglang/srt/managers/tokenizer_manager.py
ch-wan
reviewed
Feb 23, 2025
# Conflicts: # python/sglang/srt/entrypoints/engine.py # python/sglang/srt/entrypoints/http_server.py # python/sglang/srt/managers/scheduler.py
yangw1234
reviewed
Feb 23, 2025
yangw1234
approved these changes
Feb 23, 2025
|
LGTM except on minor comment. |
ch-wan
approved these changes
Feb 24, 2025
…into feat/separate_comm
ch-wan
reviewed
Feb 24, 2025
shanyu-sys
reviewed
Feb 24, 2025
Comment on lines
-1496
to
+1366
| self.send_to_detokenizer.send_pyobj( | ||
| self.on_generation_output( |
There was a problem hiding this comment.
Shall we check self.on_generation_output before using it? e.g. Add an error message if self.on_generation_output is None.
There was a problem hiding this comment.
It seems Scheduler class is an internal implementation detail and not user-facing API, so I personally feel both adding and not adding is OK.
|
After discussing with Lianmin, now I know there is no need to follow implementation requirements in e.g. 2736. Thus I spent several hours writing a new PR #3852, and this one is deprecated. |
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.
Motivation
Currently, the managers do both communication (e.g. "send to another process using mq") and computation (e.g. "do the detokenization"). This PR separates them.
Moreover, by doing this refactor, adding SPMD logic would be a bit easier and cleaner.
Modifications
Checklist