Skip to content

Improve configs - LoadConfig #16422

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Apr 11, 2025
Merged

Improve configs - LoadConfig #16422

merged 9 commits into from
Apr 11, 2025

Conversation

hmellor
Copy link
Member

@hmellor hmellor commented Apr 10, 2025 •
edited by github-actions bot
Loading

Follows #16332

Notable changes:

  • FlexibleArgumentParser has been updated to allow newlines in argument help strings. This enables the LoadFormat.load_format help string to look much nicer.
  • In the previous PR the config decorator broke the IDE type detection. This is fixed using a TypeVar.
  • The get_kwargs method in add_cli_args now identifies Union[str, ...] types. This enables types which can be combinations of:
    • str (this type must be present for the current detection to work)
    • Enum of str
    • class

@hmellor
Improve configs - LoadConfig
d418769 
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@github-actions GitHub Actions
Copy link

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

🚀

russellb
russellb requested changes Apr 10, 2025
View reviewed changes
Copy link
Member

@russellb russellb left a comment

Choose a reason for hiding this comment

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

@hmellor CI failure looks related

@hmellor
Copy link
Member Author

hmellor commented Apr 10, 2025

Yep, we haven't had to deal with Union types yet. I know how to fix it, I'll push tomorrow

hmellor added 5 commits April 11, 2025 13:19
@hmellor
Enable newlines in argparse help strings
9fc2cac 
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@hmellor
Update docstring formatting of LoadConfig.load_format
de4329a 
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@hmellor
Fix IntelliSense for config decorator
0dd13b2 
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@hmellor
Handle Union[str, ...]
2d39e64 
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@hmellor
Use Enum member value as default
4d5df23 
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@hmellor hmellor requested a review from russellb April 11, 2025 12:06
hmellor added 3 commits April 11, 2025 15:59
@hmellor
Better handling of newlines in config docstrings
fe71abc 
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@hmellor
Merge branch 'main' into improve-load-config
31dd1fa
@hmellor
Typo
ce5dd55 
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
russellb
russellb approved these changes Apr 11, 2025
View reviewed changes
Copy link
Member

@russellb russellb left a comment

Choose a reason for hiding this comment

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

lgtm assuming tests are passing now!

@hmellor hmellor added the ready ONLY add when PR is ready to merge/full CI is needed label Apr 11, 2025
@hmellor hmellor enabled auto-merge (squash) April 11, 2025 19:27
@hmellor hmellor merged commit cd77382 into vllm-project:main Apr 11, 2025
62 checks passed
@hmellor hmellor deleted the improve-load-config branch April 11, 2025 20:27
@DamonFool DamonFool mentioned this pull request Apr 12, 2025
Merged
@DamonFool
Copy link
Contributor

Hi, could you please see this pr (#16532) which fixes two warnings? @hmellor
Thanks.

This was referenced Apr 13, 2025
Merged
Merged
This was referenced Apr 17, 2025
Merged
Merged
yangw-dev pushed a commit to yangw-dev/vllm that referenced this pull request Apr 21, 2025
@hmellor @yangw-dev
Improve configs - LoadConfig (vllm-project#16422)
99b08d8 
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Yang Wang <elainewy@meta.com>
This was referenced Apr 22, 2025
Merged
Merged
Merged
jikunshang pushed a commit to jikunshang/vllm that referenced this pull request Apr 29, 2025
@hmellor @dbyoung18
Improve configs - LoadConfig (vllm-project#16422)
b166310 
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
lk-chen pushed a commit to lk-chen/vllm that referenced this pull request Apr 29, 2025
@hmellor @lk-chen
Improve configs - LoadConfig (vllm-project#16422)
b7643a9 
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
@hmellor hmellor mentioned this pull request Apr 30, 2025
Merged
RichardoMrMu pushed a commit to RichardoMrMu/vllm that referenced this pull request May 12, 2025
@hmellor
Improve configs - LoadConfig (vllm-project#16422)
cf6209a 
Signed-off-by: Harry Mellor <19981378+hmellor@users.noreply.github.com>
Signed-off-by: Mu Huai <tianbowen.tbw@antgroup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@russellb russellb russellb approved these changes

Assignees
No one assigned
Labels
ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hmellor @DamonFool @russellb