Skip to content

Default TreadPool size to number of physical cores#125963

Closed
malfet wants to merge 1 commit intomainfrom
malfet-patch-29
Closed

Default TreadPool size to number of physical cores#125963
malfet wants to merge 1 commit intomainfrom
malfet-patch-29

Conversation

@malfet
Copy link
Copy Markdown
Contributor

@malfet malfet commented May 10, 2024

TODO: Some benchmarks

@malfet malfet added the topic: performance topic category label May 10, 2024
@malfet malfet requested review from albanD and janeyx99 May 10, 2024 21:10
@pytorch-bot
Copy link
Copy Markdown

pytorch-bot bot commented May 10, 2024 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/125963

Note: Links to docs will display an error until the docs builds have been completed.

❌ 2 New Failures

As of commit be2399a with merge base bbe68a1 (image):

NEW FAILURES - The following jobs have failed:

  • pull / linux-focal-cuda12.4-py3.10-gcc9 / build (gh)
    /var/lib/jenkins/workspace/aten/src/ATen/cuda/CUDASparseDescriptors.h:119:68: error: ‘cusparseStatus_t cusparseCreateBsrsm2Info(bsrsm2Info**)’ is deprecated: The routine will be removed in the next major release [-Werror=deprecated-declarations]
  • pull / linux-focal-cuda12.4-py3.10-gcc9-sm86 / build (gh)
    /var/lib/jenkins/workspace/aten/src/ATen/cuda/CUDASparseDescriptors.h:119:68: error: ‘cusparseStatus_t cusparseCreateBsrsm2Info(bsrsm2Info**)’ is deprecated: The routine will be removed in the next major release [-Werror=deprecated-declarations]

This comment was automatically generated by Dr. CI and updates every 15 minutes.

janeyx99
janeyx99 approved these changes May 10, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@janeyx99 janeyx99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do believe this change is for the better but I'm not an expert and so I cannot ascertain that is always better. @malfet what's the plan on the benchmarks haha

@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented May 10, 2024

@janeyx99 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@Skylion007
Copy link
Copy Markdown
Collaborator

Skylion007 commented May 11, 2024

Will this work properly for partial CPU allocations on SLURM clusters?

@msaroufim
Copy link
Copy Markdown
Collaborator

msaroufim commented May 12, 2024

This might be a noop concern but was also curious how a change like this affects the performance of distributed jobs

@malfet
Copy link
Copy Markdown
Contributor Author

malfet commented May 14, 2024

Will this work properly for partial CPU allocations on SLURM clusters?

I can only hope for it :)

@Skylion007
Copy link
Copy Markdown
Collaborator

Skylion007 commented May 14, 2024

I'm just worried becaues there might be say 16 logical cores, and only 4 are available to the job meaning that the cores become over-subscribed.

@janeyx99
Copy link
Copy Markdown
Contributor

janeyx99 commented May 14, 2024

@Skylion007 that is the exact type of problem we're attempting to fix, as processors usually means threads and cores means actual cores (what we're thinking). @malfet's PR will be strictly an improvement on that front as written compared to before the change.

@gajjanag
Copy link
Copy Markdown

gajjanag commented May 14, 2024

LGTM - tested both a hyperthreaded multicore and non hyperthreaded multicore. It gives the correct thread count now.

@sanchitintel
Copy link
Copy Markdown
Collaborator

sanchitintel commented May 15, 2024 •
edited
Loading

tested both a hyperthreaded multicore and non hyperthreaded multicore. It gives the correct thread count now.

Hi @gajjanag, can you please clarify if you meant you were getting incorrect counts earlier with torch.get_num_threads?
For instance, even with HyperThreading enabled on an Intel machine, I only see physical core count with torch.get_num_threads at my end (without this patch).

Thanks!

@gajjanag
Copy link
Copy Markdown

gajjanag commented May 23, 2024

tested both a hyperthreaded multicore and non hyperthreaded multicore. It gives the correct thread count now.

Hi @gajjanag, can you please clarify if you meant you were getting incorrect counts earlier with torch.get_num_threads? For instance, even with HyperThreading enabled on an Intel machine, I only see physical core count with torch.get_num_threads at my end (without this patch).

Thanks!

Yes, I was getting incorrect counts before this patch (eg a 2 socket 56 core each Intel was giving 224 thread count, but it now gives the correct 112)

@sanchitintel
Copy link
Copy Markdown
Collaborator

sanchitintel commented May 23, 2024

Thanks for confirming, @gajjanag!

That hasn't been my experience, with HyperThreading enabled (without this patch) -
image

@malfet
Copy link
Copy Markdown
Contributor Author

malfet commented May 23, 2024

Thanks for confirming, @gajjanag!

That hasn't been my experience, with HyperThreading enabled (without this patch) - image

We need to chat about this. Perhaps cpuinfo does not correctly work on your system, but it should have returned number of logical cores rather than physical ones.

@malfet
Copy link
Copy Markdown
Contributor Author

malfet commented May 23, 2024

@pytorchbot rebase

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

pytorchmergebot commented May 23, 2024

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

pytorchmergebot commented May 23, 2024

Successfully rebased malfet-patch-29 onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout malfet-patch-29 && git pull --rebase)

@malfet malfet added the release notes: python_frontend python frontend release notes category label May 24, 2024
@malfet
Copy link
Copy Markdown
Contributor Author

malfet commented May 24, 2024

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label May 24, 2024
@pytorchmergebot
Copy link
Copy Markdown
Collaborator

pytorchmergebot commented May 24, 2024

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

pytorchmergebot commented May 24, 2024

Merge failed

Reason: 2 mandatory check(s) failed. The first few are:

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

Failing merge rule: Core Maintainers

@malfet
Copy link
Copy Markdown
Contributor Author

malfet commented May 24, 2024

@pytorchbot merge -i

@pytorchmergebot
Copy link
Copy Markdown
Collaborator

pytorchmergebot commented May 24, 2024

Merge started

Your change will be merged while ignoring the following 3 checks: pull / linux-focal-cuda12.4-py3.10-gcc9 / build, pull / linux-focal-cuda12.4-py3.10-gcc9-sm86 / build, Meta Internal-Only Changes Check

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@huydhn huydhn mentioned this pull request May 24, 2024
Closed
titaiwangms pushed a commit to titaiwangms/pytorch that referenced this pull request May 28, 2024
@malfet @titaiwangms
Default TreadPool size to number of physical cores (pytorch#125963)
b89886b 
TODO: Some benchmarks

Pull Request resolved: pytorch#125963
Approved by: https://github.com/janeyx99, https://github.com/Skylion007, https://github.com/gajjanag, https://github.com/jgong5
@github-actions github-actions bot deleted the malfet-patch-29 branch June 25, 2024 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Skylion007 Skylion007 Skylion007 approved these changes

@jgong5 jgong5 jgong5 approved these changes

@janeyx99 janeyx99 janeyx99 approved these changes

@albanD albanD Awaiting requested review from albanD

+1 more reviewer

@gajjanag gajjanag gajjanag approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged release notes: python_frontend python frontend release notes category topic: performance topic category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@malfet @facebook-github-bot @Skylion007 @msaroufim @janeyx99 @gajjanag @sanchitintel @pytorchmergebot @jgong5