Skip to content
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

[RFC]: More rigorous compilation warnings #6994

Closed
tlrmchlsmth opened this issue Jul 31, 2024 · 3 comments
Closed

[RFC]: More rigorous compilation warnings #6994

tlrmchlsmth opened this issue Jul 31, 2024 · 3 comments

Comments

@tlrmchlsmth
Copy link
Collaborator

tlrmchlsmth commented Jul 31, 2024

Motivation.

For this RFC, I want to tighten down vLLM's compiler flags regarding warnings. Both to eliminate warnings when compiling code, and also to increase the number of warnings to prevent certain classes of bugs that have proven to be problematic in vLLM.

Why eliminate warnings? Seeing a lot of warning spam is annoying. Bogus or unimportant warnings can also mask real problems. It also just looks bad -- until #6904, one of the first things developers saw when building vLLM from source was a ton of spam about possible divide-by-zeros. These things are usually easy to fix and just need a small forcing function to make it happen.

Why add more warnings? I think we can eliminate or drastically reduce certain classes of bugs. For example, -Wconvert will hopefully prevent bugs like those fixed in #6838, #6649, and #1514 by making our narrowing conversions explicit.

Proposed Change.

Turn on -Werror
This is necessary in order to enforce a clean build.

Turn on -Wconversion
Eliminate narrowing conversion bugs especially from int64_t to int32_t. Currently way too spammy to turn on, especially in the attention kernels.

Feedback Period.

No response

CC List.

@bnellnm @WoosukKwon

Any Other Things.

No response

@tlrmchlsmth
Copy link
Collaborator Author

Just for reference, this is what we've used in the past at Neural Magic. Although this is definitely more strict than we want to use in vLLM at least to start with, and some of this came from jumping through hoops to compile on both g++ and clang++ without warnings:

WERROR_FLAG_DEF	= -Werror -Wno-error=deprecated-declarations
NO_WARN := -Wno-unused-parameter -Wno-unknown-pragmas -Wno-parentheses -Wno-missing-braces -Wno-attributes -Wno-nonnull
CXXWARN		=	-Wall -Wextra $(WERROR_FLAG) $(NO_WARN)

@tlrmchlsmth tlrmchlsmth changed the title [RFC]: More rigorous compile warnings [RFC]: More rigorous compilation warnings Jul 31, 2024
Copy link

github-actions bot commented Nov 1, 2024

This issue has been automatically marked as stale because it has not had any activity within 90 days. It will be automatically closed if no further activity occurs within 30 days. Leave a comment if you feel this issue should remain open. Thank you!

@github-actions github-actions bot added the stale label Nov 1, 2024
Copy link

github-actions bot commented Dec 1, 2024

This issue has been automatically closed due to inactivity. Please feel free to reopen if you feel it is still relevant. Thank you!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant