Skip to content

[RFC] [Misc] Convert untyped OmniStageTask dict to TypedDict#1547

Closed
NickCao wants to merge 5 commits into
vllm-project:mainfrom
NickCao:typed-dict
Closed

[RFC] [Misc] Convert untyped OmniStageTask dict to TypedDict#1547
NickCao wants to merge 5 commits into
vllm-project:mainfrom
NickCao:typed-dict

Conversation

@NickCao
Copy link
Copy Markdown
Contributor

@NickCao NickCao commented Feb 27, 2026

PLEASE FILL IN THE PR DESCRIPTION HERE ENSURING ALL CHECKLIST ITEMS (AT THE BOTTOM) HAVE BEEN CONSIDERED.

Purpose

Currently the task messages passed between stages are untyped dict, requiring the receiving stage to validate all incoming messages manually. Thus I propose we gradually type them as TypedDict, and eventually as pydantic tagged union to improve the maintainability of these messages.

Test Plan

TypedDicts are just regular dicts at runtime, no additional testing is required, apart from testing if the stages still work.

Test Result

N/A


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan. Please provide the test scripts & test commands. Please state the reasons if your codes don't require additional test scripts. For test file guidelines, please check the test style doc
  • The test results. Please paste the results comparison before and after, or the e2e results.
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model. Please run mkdocs serve to sync the documentation editions to ./docs.
  • (Optional) Release notes update. If your change is user-facing, please update the release notes draft.

BEFORE SUBMITTING, PLEASE READ https://github.com/vllm-project/vllm-omni/blob/main/CONTRIBUTING.md (anything written below this line will be removed by GitHub Actions)

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7f2d3b0440

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread vllm_omni/entrypoints/stage_utils.py Outdated
Comment thread vllm_omni/entrypoints/stage_utils.py
Comment thread vllm_omni/entrypoints/async_omni.py
Comment thread vllm_omni/distributed/omni_connectors/adapter.py
Copy link
Copy Markdown
Collaborator

@lishunyang12 lishunyang12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple comments. Main concern is that OmniStageTaskGenerate tries to cover both seed tasks and connector notifications, which have almost entirely different field sets. The most important construction sites (stage-0 seeds in omni.py and async_omni.py) are still raw dicts, which kind of defeats the purpose of the typing effort.

@NickCao
Copy link
Copy Markdown
Contributor Author

NickCao commented Mar 2, 2026

The most important construction sites (stage-0 seeds in omni.py and async_omni.py) are still raw dicts, which kind of defeats the purpose of the typing effort.

Converted these to OmniStageTaskGenerate, whether to further factor them into a new task type can happen in a followup PR.

@NickCao NickCao requested a review from lishunyang12 March 3, 2026 14:07
@NickCao NickCao force-pushed the typed-dict branch 3 times, most recently from 85ca3cb to 3a26869 Compare March 4, 2026 13:37
@lishunyang12
Copy link
Copy Markdown
Collaborator

Seed-site conversions look good. Two things to fix:

  1. sampling_params is declared twice in OmniStageTaskGenerate — second definition shadows the first. Pick one type (probably NotRequired[Any]).
  2. connector_metadata is still dynamically assigned in adapter.py but missing from the TypedDict.

Copy link
Copy Markdown
Collaborator

@lishunyang12 lishunyang12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See reviews

@NickCao
Copy link
Copy Markdown
Contributor Author

NickCao commented Mar 4, 2026

See reviews

Already resolved in the latest push?

NickCao added 5 commits March 10, 2026 11:06
…Dict

Signed-off-by: Nick Cao <ncao@redhat.com>
…TOP} to TypedDict

Signed-off-by: Nick Cao <ncao@redhat.com>
…Dict

Signed-off-by: Nick Cao <ncao@redhat.com>
Signed-off-by: Nick Cao <ncao@redhat.com>
@NickCao
Copy link
Copy Markdown
Contributor Author

NickCao commented Mar 10, 2026

Rebased on current main.

@Gaohan123
Copy link
Copy Markdown
Collaborator

@natureofnature PTAL

@Gaohan123 Gaohan123 added this to the v0.18.0 milestone Mar 14, 2026
@hsliuustc0106
Copy link
Copy Markdown
Collaborator

fix conflicts

@NickCao
Copy link
Copy Markdown
Contributor Author

NickCao commented Mar 22, 2026

No longer relevant after #1908

@NickCao NickCao closed this Mar 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants