-
Notifications
You must be signed in to change notification settings - Fork 863
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
Enable worker core pinning in CPU nightly benchmark #2166
Enable worker core pinning in CPU nightly benchmark #2166
Conversation
…om/min-jean-cho/serve into minjean/enable_worker_core_pinning
Just retriggered CI - @min-jean-cho I noticed CI failures last week and a bunch of upstream changes to torch, do you need any help resolving this PR? |
Thanks @msaroufim, let me convert this PR to WIP. The current status of this PR is to address the concerns brought up by @lxning when deploying this in production. I'll update this when ready, thanks! |
Codecov Report
@@ Coverage Diff @@
## master #2166 +/- ##
=======================================
Coverage 71.41% 71.41%
=======================================
Files 73 73
Lines 3348 3348
Branches 57 57
=======================================
Hits 2391 2391
Misses 954 954
Partials 3 3 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
def install_numactl(self): | ||
if os.system("numactl --show") != 0 or args.force: | ||
os.system(f"{self.sudo_cmd}apt-get install -y numactl") | ||
|
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.
do we need add numactl into docker container for cpu?
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.
Same question as the one Li asked, otherwise LGTM. Approving because the this isn't breaking the docker CI
In our current dev docker images we do indeed run the install dependencies script https://github.com/pytorch/serve/blob/master/docker/Dockerfile.dev#L70 but since we're moving to using the regular docker images moving forward should also add numactl there - cc @agunapal
We can add |
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.
@min-jean-cho Could you please tell me where numactl is being used here.
Trying to understand why docker CI is not complaining.
@agunapal
I think because if |
This PR enables worker core pinning by default for better cpu performance.
Below is a nightly cpu benchmark performance comparison:
default
: https://github.com/pytorch/serve/actions/runs/4238841923OMP_NUM_THREADS=1
: https://github.com/pytorch/serve/actions/runs/4255376669core pinning, --use_logical_cores
: https://github.com/pytorch/serve/actions/runs/4320222197