Centralized environment variables #1058
Conversation
xingliu14
force-pushed
the
env_var
branch
2 times, most recently
from
85aa68d to
38a2547
Compare
|
Thanks! So this PR only creates env.py file? But this PR does make changes so that our existing code make use of this new file right? Can you also add that feature and everything works correctly? |
xingliu14
force-pushed
the
env_var
branch
2 times, most recently
from
3c07b53 to
737eed5
Compare
|
I am planning to create the env file for review and gradually refactor the env var usages. I replaced |
|
Personally, I.e., you can test out the feature with Can you apply this feature into |
|
Replaced |
|
Thank you, will approve when the CI passes: https://buildkite.com/tpu-commons/tpu-inference-ci/builds/5201 |
|
CI looks good, but please resolve merge conflicts. Also, please remove |
- Fix boolean environment variables: use bool(int(...)) instead of bool(os.getenv(..., False)) - Correctly converts '0' to False and '1' to True - Fixed JAX_RANDOM_WEIGHTS, SKIP_JAX_PRECOMPILE, NEW_MODEL_DESIGN - Standardize to use os.getenv() consistently across all variables - Improve docstrings: - Clarify __getattr__ comment: function is wrapped with functools.cache() - Clarify enable_envs_cache() comment: explains caching behavior and lifecycle - Type annotations in TYPE_CHECKING now accurately reflect runtime behavior Signed-off-by: Xing Liu <xingliu14@gmail.com>
Signed-off-by: Xing Liu <xingliu14@gmail.com>
Signed-off-by: Xing Liu <xingliu14@gmail.com>
Signed-off-by: Xing Liu <xingliu14@gmail.com>
Signed-off-by: Xing Liu <xingliu14@gmail.com>
|
Done. |
|
Thanks! Running CI: https://buildkite.com/tpu-commons/tpu-inference-ci/builds/5238 Let me know when it's finished. I'll give approval when it all looks good. |
|
The CI failed, while the main branch CI also failed for the same tests, I may wait until the error be fixed. |
| } | ||
|
|
||
|
|
||
| def __getattr__(name: str) -> Any: |
There was a problem hiding this comment.
can we add test coverage like what vllm.main do?
https://github.com/vllm-project/vllm/blob/main/tests/test_envs.py
There was a problem hiding this comment.
good idea. I initially thought that existing unit test that utilizes these environment variables has passed as a sufficient coverage test. But it's probably a good idea to add a separate test as well.
@xingliu14, can you add a unit test like the one reference here?
There was a problem hiding this comment.
Sure, I'll add test on the follow up PR.
|
Please take this as example and add test coverage for envs.py |
3 participants
Description
Created envs.py to centrally store environment variables.
Replaced
MODEL_IMPL_TYPEandUSE_MOE_EP_KERNELusages to use the envs.Addressing #1016.
Tests
pytest /home/liuxing_google_com/tpu-inference/tests/worker/tpu_worker_test.py -vpytest /home/liuxing_google_com/tpu-inference/tests/models/common/test_model_loader.py -vChecklist
Before submitting this PR, please make sure: