-
Notifications
You must be signed in to change notification settings - Fork 26
Only set threading env vars if not already defined #483
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
Only set threading env vars if not already defined #483
Conversation
👋 Hi! Thank you for contributing to vLLM support on Spyre.
Or this can be done with
Now you are good to go 🚀 |
hey @AnishPahilajani please sign off your commit correctly and run our linter (I assume it will fail as I see a double space after the if) |
vllm_spyre/platform.py
Outdated
|
||
for env in THREADING_ENVS: | ||
os.environ[env] = str(cpus_per_worker) | ||
if not os.environ.get(env): |
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.
double space?
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.
fixed
The (We actually have a case where a script in an image sets OMP_NUM_THREADS automatically, but it can't take the tensor-parallelism of vLLM into account. So I'd prefer to just have the feature flag instead of the feature flag + override if not set) |
Signed-off-by: Anish Pahilajani <[email protected]>
9e95b4f
to
82f7a3d
Compare
I understand the idea behind using only the On our Power 11 systems, some threading env vars (like OMP_NUM_THREADS) are set by scripts in the image, while others are unset. If we override all vars unconditionally, it could break existing configurations. That’s why I added the conditional check ( I also expect that scenario (script setting OMP_NUM_THREADS) to still work correctly. |
On the other hand, the "existing configuration" can be sub-optimal for or break vllm-spyre. What if you want to override the system script's My thought with |
# Description A couple of improvements related to setting threading based on cpu count: - add `VLLM_SPYRE_NUM_CPUS` configuration to set the cpu count and skip the detection steps - this is useful if detection is not working but you want just 1 configuration to set and to still have vllm-spyre scale by the worker count - adds `psutil` as another way to auto-detect CPUs by counting "physical" cores only instead of logical cores - in multi-threaded CPU bottlenecks, using logical cores may be inefficient - cpu detection is meant to be best-effort so I didn't add psutil as a dependency (though it does currently come in through the accelerate sub-dependency of the `fp8` extras package) ## Related Issues #483 --------- Signed-off-by: Travis Johnson <[email protected]>
closing as we have an alternative implemented in #487 |
Description
This PR ensures that threading-related environment variables (e.g. OPENBLAS_NUM_THREADS, MKL_NUM_THREADS, etc.) are only set if they are not already defined by the user.