-
Notifications
You must be signed in to change notification settings - Fork 26
♻️ Simplify env var overrides and add tests #525
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Joe Runde <[email protected]>
👋 Hi! Thank you for contributing to vLLM support on Spyre.
Or this can be done with
Now you are good to go 🚀 |
Signed-off-by: Joe Runde <[email protected]>
bot:test |
Signed-off-by: Joe Runde <[email protected]>
Signed-off-by: Joe Runde <[email protected]>
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
envs.override("VLLM_SPYRE_NOT_A_CONFIG", "nothing") | ||
|
||
|
||
def test_sendnn_decoder_backwards_compat(monkeypatch): |
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.
🌶️ 🌶️ Test simplification
Description
There seems to be a small bug where passing a valid env var override will not set the correct value in the environment, only the cache. This could be an issue where the env var needs to be set to be propagated across afork
, but currently we only use it for updating the batching mode for poolers so this shouldn't be a big issue atm.Edit: forking would use the cache anyway so this shouldn't be a bug. But the logic for handling the environment during overrides was a bit funky- this PR simplifies that code and adds explicit tests covering the expected behavior.
This PR fixes that, adding some tests to cover our
envs
package. Since there was an expensive e2e test covering thesendnn_decoder
backwards compatibility, I moved it to this new test file so that we only have to test the config override instead of loading and running a whole model.