-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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.
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
Rename vllm.logging to vllm.logging_utils #10134
Conversation
👋 Hi! Thank you for contributing to the vLLM project. 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 do one of these:
🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit message needs a Signed-off-by
header to pass the DCO check.
As for the changes, they look correct, though I'm not sure this is worth fixing. Avoiding naming collisions is an impossible task if we want to avoid all possible collisions to allow running code from within any arbitrary vllm source directory.
In any case, that's just an opinion and it's not my call to make.
|
||
__all__ = [ | ||
"NewLineFormatter", | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be ideal if this file was a git mv
as well, like formatter.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the content changes from vllm.logging
to vllm.logging_utils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only 1 line changes, not the whole file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, it's correct in git, and just not displayed like that in github's diff viewer.
@youkaichao FYI, since you filed the corresponding issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
we only do it in case-by-case, only do it when we find annoying cases. |
Signed-off-by: Loc Huynh <[email protected]>
Signed-off-by: Jee Jee Li <[email protected]>
Signed-off-by: Sumit Dubey <[email protected]>
`logging_configuration.md` provides some sample logger configuration that is no logner valid after vllm-project#10134. This PR fixes the sample config, as well as provides backwards compatibility so that existing configuration files won't break. Closes vllm-project#10457 Signed-off-by: Russell Bryant <[email protected]>
Signed-off-by: Maxime Fournioux <[email protected]>
`logging_configuration.md` provides some sample logger configuration that is no logner valid after vllm-project#10134. This PR fixes the sample config, as well as provides backwards compatibility so that existing configuration files won't break. Closes vllm-project#10457 Signed-off-by: Russell Bryant <[email protected]>
Signed-off-by: Tyler Michael Smith <[email protected]>
Hopefully Fixes #10133
Done from mobile, untested at the moment