delegate parallelism to Ninja when possible #64733

dagitses · 2021-09-09T09:00:15Z

Stack from ghstack:

The previous implementation was wrong when CPU scheduling affinity is
set. In fact, it is still wrong if Ninja is not being used.

When there is CPU scheduling affinity set, the number of processors
available on the system likely exceeds the number of processors that
are usable to the build. We ought to use
len(os.sched_getaffinity(0)) to determine the effective parallelism.

This change is more minimal and instead just delegates to Ninja (which
handles this correctly) when it is used.

Test Plan:
I verified this worked as correctly using Ninja on a 96-core machine
with 24 cores available for scheduling by checking:

the cmake command did not specify "-j"
the number of top-level jobs in top/pstree never exceeded 26 (24 +
2)

And I verified we get the legacy behavior by specifying USE_NINJA=0 on
the build.

And I also verified that MAX_JOBS trumps everything if specified.

Differential Revision: D30968796

The previous implementation was wrong when CPU scheduling affinity is set. In fact, it is still wrong if Ninja is not being used. When there is CPU scheduling affinity set, the number of processors available on the system likely exceeds the number of processors that are usable to the build. We ought to use `len(os.sched_getaffinity(0))` to determine the effective parallelism. This change is more minimal and instead just delegates to Ninja (which handles this correctly) when it is used. Test Plan: I verified this worked as correctly using Ninja on a 96-core machine with 24 cores available for scheduling by checking: * the cmake command did not specify "-j" * the number of top-level jobs in top/pstree never exceeded 26 (24 + 2) [ghstack-poisoned]

facebook-github-bot · 2021-09-09T09:00:21Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/64733
📄 Preview docs built from this PR
📄 Preview C++ docs built from this PR
🔧 Opt-in to CIFlow to control what jobs run on your PRs

💊 CI failures summary and remediations

As of commit 2e28130 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

The previous implementation was wrong when CPU scheduling affinity is set. In fact, it is still wrong if Ninja is not being used. When there is CPU scheduling affinity set, the number of processors available on the system likely exceeds the number of processors that are usable to the build. We ought to use `len(os.sched_getaffinity(0))` to determine the effective parallelism. This change is more minimal and instead just delegates to Ninja (which handles this correctly) when it is used. Test Plan: I verified this worked as correctly using Ninja on a 96-core machine with 24 cores available for scheduling by checking: * the cmake command did not specify "-j" * the number of top-level jobs in top/pstree never exceeded 26 (24 + 2) [ghstack-poisoned]

The previous implementation was wrong when CPU scheduling affinity is set. In fact, it is still wrong if Ninja is not being used. When there is CPU scheduling affinity set, the number of processors available on the system likely exceeds the number of processors that are usable to the build. We ought to use `len(os.sched_getaffinity(0))` to determine the effective parallelism. This change is more minimal and instead just delegates to Ninja (which handles this correctly) when it is used. Test Plan: I verified this worked as correctly using Ninja on a 96-core machine with 24 cores available for scheduling by checking: * the cmake command did not specify "-j" * the number of top-level jobs in top/pstree never exceeded 26 (24 + 2) And I verified we get the legacy behavior by specifying USE_NINJA=0 on the build. [ghstack-poisoned]

The previous implementation was wrong when CPU scheduling affinity is set. In fact, it is still wrong if Ninja is not being used. When there is CPU scheduling affinity set, the number of processors available on the system likely exceeds the number of processors that are usable to the build. We ought to use `len(os.sched_getaffinity(0))` to determine the effective parallelism. This change is more minimal and instead just delegates to Ninja (which handles this correctly) when it is used. Test Plan: I verified this worked as correctly using Ninja on a 96-core machine with 24 cores available for scheduling by checking: * the cmake command did not specify "-j" * the number of top-level jobs in top/pstree never exceeded 26 (24 + 2) And I verified we get the legacy behavior by specifying USE_NINJA=0 on the build. ghstack-source-id: 3ed3bf6 Pull Request resolved: #64733

tools/setup_helpers/cmake.py

The previous implementation was wrong when CPU scheduling affinity is set. In fact, it is still wrong if Ninja is not being used. When there is CPU scheduling affinity set, the number of processors available on the system likely exceeds the number of processors that are usable to the build. We ought to use `len(os.sched_getaffinity(0))` to determine the effective parallelism. This change is more minimal and instead just delegates to Ninja (which handles this correctly) when it is used. Test Plan: I verified this worked as correctly using Ninja on a 96-core machine with 24 cores available for scheduling by checking: * the cmake command did not specify "-j" * the number of top-level jobs in top/pstree never exceeded 26 (24 + 2) And I verified we get the legacy behavior by specifying USE_NINJA=0 on the build. [ghstack-poisoned]

The previous implementation was wrong when CPU scheduling affinity is set. In fact, it is still wrong if Ninja is not being used. When there is CPU scheduling affinity set, the number of processors available on the system likely exceeds the number of processors that are usable to the build. We ought to use `len(os.sched_getaffinity(0))` to determine the effective parallelism. This change is more minimal and instead just delegates to Ninja (which handles this correctly) when it is used. Test Plan: I verified this worked as correctly using Ninja on a 96-core machine with 24 cores available for scheduling by checking: * the cmake command did not specify "-j" * the number of top-level jobs in top/pstree never exceeded 26 (24 + 2) And I verified we get the legacy behavior by specifying USE_NINJA=0 on the build. ghstack-source-id: c3cc538 Pull Request resolved: #64733

dagitses · 2021-09-15T19:18:06Z

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

The previous implementation was wrong when CPU scheduling affinity is set. In fact, it is still wrong if Ninja is not being used. When there is CPU scheduling affinity set, the number of processors available on the system likely exceeds the number of processors that are usable to the build. We ought to use `len(os.sched_getaffinity(0))` to determine the effective parallelism. This change is more minimal and instead just delegates to Ninja (which handles this correctly) when it is used. Test Plan: I verified this worked as correctly using Ninja on a 96-core machine with 24 cores available for scheduling by checking: * the cmake command did not specify "-j" * the number of top-level jobs in top/pstree never exceeded 26 (24 + 2) And I verified we get the legacy behavior by specifying USE_NINJA=0 on the build. Differential Revision: [D30968796](https://our.internmc.facebook.com/intern/diff/D30968796) [ghstack-poisoned]

dagitses · 2021-09-16T02:48:52Z

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

The previous implementation was wrong when CPU scheduling affinity is set. In fact, it is still wrong if Ninja is not being used. When there is CPU scheduling affinity set, the number of processors available on the system likely exceeds the number of processors that are usable to the build. We ought to use `len(os.sched_getaffinity(0))` to determine the effective parallelism. This change is more minimal and instead just delegates to Ninja (which handles this correctly) when it is used. Test Plan: I verified this worked as correctly using Ninja on a 96-core machine with 24 cores available for scheduling by checking: * the cmake command did not specify "-j" * the number of top-level jobs in top/pstree never exceeded 26 (24 + 2) And I verified we get the legacy behavior by specifying USE_NINJA=0 on the build. And I also verified that MAX_JOBS trumps everything if specified. Differential Revision: [D30968796](https://our.internmc.facebook.com/intern/diff/D30968796) [ghstack-poisoned]

The previous implementation was wrong when CPU scheduling affinity is set. In fact, it is still wrong if Ninja is not being used. When there is CPU scheduling affinity set, the number of processors available on the system likely exceeds the number of processors that are usable to the build. We ought to use `len(os.sched_getaffinity(0))` to determine the effective parallelism. This change is more minimal and instead just delegates to Ninja (which handles this correctly) when it is used. Test Plan: I verified this worked as correctly using Ninja on a 96-core machine with 24 cores available for scheduling by checking: * the cmake command did not specify "-j" * the number of top-level jobs in top/pstree never exceeded 26 (24 + 2) And I verified we get the legacy behavior by specifying USE_NINJA=0 on the build. And I also verified that MAX_JOBS trumps everything if specified. ghstack-source-id: 752dcd0 Pull Request resolved: #64733

The previous implementation was wrong when CPU scheduling affinity is set. In fact, it is still wrong if Ninja is not being used. When there is CPU scheduling affinity set, the number of processors available on the system likely exceeds the number of processors that are usable to the build. We ought to use `len(os.sched_getaffinity(0))` to determine the effective parallelism. This change is more minimal and instead just delegates to Ninja (which handles this correctly) when it is used. Test Plan: I verified this worked as correctly using Ninja on a 96-core machine with 24 cores available for scheduling by checking: * the cmake command did not specify "-j" * the number of top-level jobs in top/pstree never exceeded 26 (24 + 2) And I verified we get the legacy behavior by specifying USE_NINJA=0 on the build. And I also verified that MAX_JOBS trumps everything if specified. Differential Revision: [D30968796](https://our.internmc.facebook.com/intern/diff/D30968796) [ghstack-poisoned]

The previous implementation was wrong when CPU scheduling affinity is set. In fact, it is still wrong if Ninja is not being used. When there is CPU scheduling affinity set, the number of processors available on the system likely exceeds the number of processors that are usable to the build. We ought to use `len(os.sched_getaffinity(0))` to determine the effective parallelism. This change is more minimal and instead just delegates to Ninja (which handles this correctly) when it is used. Test Plan: I verified this worked as correctly using Ninja on a 96-core machine with 24 cores available for scheduling by checking: * the cmake command did not specify "-j" * the number of top-level jobs in top/pstree never exceeded 26 (24 + 2) And I verified we get the legacy behavior by specifying USE_NINJA=0 on the build. And I also verified that MAX_JOBS trumps everything if specified. ghstack-source-id: b9e932c Pull Request resolved: #64733

dagitses · 2021-09-16T04:09:09Z

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

The previous implementation was wrong when CPU scheduling affinity is set. In fact, it is still wrong if Ninja is not being used. When there is CPU scheduling affinity set, the number of processors available on the system likely exceeds the number of processors that are usable to the build. We ought to use `len(os.sched_getaffinity(0))` to determine the effective parallelism. This change is more minimal and instead just delegates to Ninja (which handles this correctly) when it is used. Test Plan: I verified this worked as correctly using Ninja on a 96-core machine with 24 cores available for scheduling by checking: * the cmake command did not specify "-j" * the number of top-level jobs in top/pstree never exceeded 26 (24 + 2) And I verified we get the legacy behavior by specifying USE_NINJA=0 on the build. And I also verified that MAX_JOBS trumps everything if specified. Differential Revision: [D30968796](https://our.internmc.facebook.com/intern/diff/D30968796) [ghstack-poisoned]

The previous implementation was wrong when CPU scheduling affinity is set. In fact, it is still wrong if Ninja is not being used. When there is CPU scheduling affinity set, the number of processors available on the system likely exceeds the number of processors that are usable to the build. We ought to use `len(os.sched_getaffinity(0))` to determine the effective parallelism. This change is more minimal and instead just delegates to Ninja (which handles this correctly) when it is used. Test Plan: I verified this worked as correctly using Ninja on a 96-core machine with 24 cores available for scheduling by checking: * the cmake command did not specify "-j" * the number of top-level jobs in top/pstree never exceeded 26 (24 + 2) And I verified we get the legacy behavior by specifying USE_NINJA=0 on the build. And I also verified that MAX_JOBS trumps everything if specified. ghstack-source-id: 03c51be Pull Request resolved: #64733

The previous implementation was wrong when CPU scheduling affinity is set. In fact, it is still wrong if Ninja is not being used. When there is CPU scheduling affinity set, the number of processors available on the system likely exceeds the number of processors that are usable to the build. We ought to use `len(os.sched_getaffinity(0))` to determine the effective parallelism. This change is more minimal and instead just delegates to Ninja (which handles this correctly) when it is used. Test Plan: I verified this worked as correctly using Ninja on a 96-core machine with 24 cores available for scheduling by checking: * the cmake command did not specify "-j" * the number of top-level jobs in top/pstree never exceeded 26 (24 + 2) And I verified we get the legacy behavior by specifying USE_NINJA=0 on the build. And I also verified that MAX_JOBS trumps everything if specified. Differential Revision: [D30968796](https://our.internmc.facebook.com/intern/diff/D30968796) [ghstack-poisoned]

dagitses · 2021-09-16T16:00:42Z

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

The previous implementation was wrong when CPU scheduling affinity is set. In fact, it is still wrong if Ninja is not being used. When there is CPU scheduling affinity set, the number of processors available on the system likely exceeds the number of processors that are usable to the build. We ought to use `len(os.sched_getaffinity(0))` to determine the effective parallelism. This change is more minimal and instead just delegates to Ninja (which handles this correctly) when it is used. Test Plan: I verified this worked as correctly using Ninja on a 96-core machine with 24 cores available for scheduling by checking: * the cmake command did not specify "-j" * the number of top-level jobs in top/pstree never exceeded 26 (24 + 2) And I verified we get the legacy behavior by specifying USE_NINJA=0 on the build. And I also verified that MAX_JOBS trumps everything if specified. Differential Revision: [D30968796](https://our.internmc.facebook.com/intern/diff/D30968796) [ghstack-poisoned]

tools/test/test_cmake.py

The previous implementation was wrong when CPU scheduling affinity is set. In fact, it is still wrong if Ninja is not being used. When there is CPU scheduling affinity set, the number of processors available on the system likely exceeds the number of processors that are usable to the build. We ought to use `len(os.sched_getaffinity(0))` to determine the effective parallelism. This change is more minimal and instead just delegates to Ninja (which handles this correctly) when it is used. Test Plan: I verified this worked as correctly using Ninja on a 96-core machine with 24 cores available for scheduling by checking: * the cmake command did not specify "-j" * the number of top-level jobs in top/pstree never exceeded 26 (24 + 2) And I verified we get the legacy behavior by specifying USE_NINJA=0 on the build. And I also verified that MAX_JOBS trumps everything if specified. Differential Revision: [D30968796](https://our.internmc.facebook.com/intern/diff/D30968796) [ghstack-poisoned]

The previous implementation was wrong when CPU scheduling affinity is set. In fact, it is still wrong if Ninja is not being used. When there is CPU scheduling affinity set, the number of processors available on the system likely exceeds the number of processors that are usable to the build. We ought to use `len(os.sched_getaffinity(0))` to determine the effective parallelism. This change is more minimal and instead just delegates to Ninja (which handles this correctly) when it is used. Test Plan: I verified this worked as correctly using Ninja on a 96-core machine with 24 cores available for scheduling by checking: * the cmake command did not specify "-j" * the number of top-level jobs in top/pstree never exceeded 26 (24 + 2) And I verified we get the legacy behavior by specifying USE_NINJA=0 on the build. And I also verified that MAX_JOBS trumps everything if specified. ghstack-source-id: 5061a5f Pull Request resolved: #64733

dagitses · 2021-09-16T18:43:02Z

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

The previous implementation was wrong when CPU scheduling affinity is set. In fact, it is still wrong if Ninja is not being used. When there is CPU scheduling affinity set, the number of processors available on the system likely exceeds the number of processors that are usable to the build. We ought to use `len(os.sched_getaffinity(0))` to determine the effective parallelism. This change is more minimal and instead just delegates to Ninja (which handles this correctly) when it is used. Test Plan: I verified this worked as correctly using Ninja on a 96-core machine with 24 cores available for scheduling by checking: * the cmake command did not specify "-j" * the number of top-level jobs in top/pstree never exceeded 26 (24 + 2) And I verified we get the legacy behavior by specifying USE_NINJA=0 on the build. And I also verified that MAX_JOBS trumps everything if specified. Differential Revision: [D30968796](https://our.internmc.facebook.com/intern/diff/D30968796) [ghstack-poisoned]

The previous implementation was wrong when CPU scheduling affinity is set. In fact, it is still wrong if Ninja is not being used. When there is CPU scheduling affinity set, the number of processors available on the system likely exceeds the number of processors that are usable to the build. We ought to use `len(os.sched_getaffinity(0))` to determine the effective parallelism. This change is more minimal and instead just delegates to Ninja (which handles this correctly) when it is used. Test Plan: I verified this worked as correctly using Ninja on a 96-core machine with 24 cores available for scheduling by checking: * the cmake command did not specify "-j" * the number of top-level jobs in top/pstree never exceeded 26 (24 + 2) And I verified we get the legacy behavior by specifying USE_NINJA=0 on the build. And I also verified that MAX_JOBS trumps everything if specified. ghstack-source-id: 3457894 Pull Request resolved: #64733

dagitses · 2021-09-17T14:22:49Z

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

facebook-github-bot · 2021-09-17T19:28:54Z

@dagitses merged this pull request in 047e682.

cloudhan · 2021-09-22T03:10:58Z

@dagitses @malfet @driazati

This PR causes building failure on windows when building with

$env:MAX_JOBS="15"
python setup.py develop

with the failure as:

cmake --build . --target install --config Release -- /p:CL_MPCount=15
ninja: error: unknown target '/p:CL_MPCount=15'

See comment
047e682#r56870845

@cloudhan

Reported by @cloudhan in #64733 (comment)

dagitses · 2021-09-22T17:59:51Z

@dagitses @malfet @driazati

This PR causes building failure on windows when building with
$env:MAX_JOBS="15"
python setup.py develop
with the failure as:
cmake --build . --target install --config Release -- /p:CL_MPCount=15
ninja: error: unknown target '/p:CL_MPCount=15'
See comment
047e682#r56870845

thanks! @malfet is fixing in #65444.

Summary: Reported by cloudhan in #64733 (comment) Fixes regression introduced by 047e682 cc malfet seemethere Pull Request resolved: #65444 Reviewed By: dagitses, seemethere Differential Revision: D31103260 Pulled By: malfet fbshipit-source-id: 9d5454a64cb8a0b96264119cf16582cc5afed284

Summary: Reported by cloudhan in pytorch#64733 (comment) Fixes regression introduced by pytorch@047e682 cc malfet seemethere Pull Request resolved: pytorch#65444 Reviewed By: dagitses, seemethere Differential Revision: D31103260 Pulled By: malfet fbshipit-source-id: 9d5454a64cb8a0b96264119cf16582cc5afed284

Summary: Reported by cloudhan in #64733 (comment) Fixes regression introduced by 047e682 cc malfet seemethere Pull Request resolved: #65444 Reviewed By: dagitses, seemethere Differential Revision: D31103260 Pulled By: malfet fbshipit-source-id: 9d5454a64cb8a0b96264119cf16582cc5afed284

facebook-github-bot added the cla signed label Sep 9, 2021

dagitses marked this pull request as ready for review September 9, 2021 12:06

dagitses requested a review from malfet September 9, 2021 17:54

driazati reviewed Sep 14, 2021

View reviewed changes

tools/setup_helpers/cmake.py Outdated Show resolved Hide resolved

dagitses requested a review from driazati September 15, 2021 18:56

driazati approved these changes Sep 15, 2021

View reviewed changes

driazati reviewed Sep 16, 2021

View reviewed changes

tools/test/test_cmake.py Outdated Show resolved Hide resolved

driazati approved these changes Sep 16, 2021

View reviewed changes

dagitses mentioned this pull request Sep 16, 2021

add test for number of jobs when building #65162

Closed

facebook-github-bot closed this in 047e682 Sep 17, 2021

facebook-github-bot added the Merged label Sep 17, 2021

facebook-github-bot deleted the gh/dagitses/27/head branch September 21, 2021 14:17

malfet added a commit that referenced this pull request Sep 22, 2021

Fix Windows ninja builds when MAX_JOBS is specified

fcbe48b

Reported by @cloudhan in #64733 (comment)

malfet mentioned this pull request Sep 22, 2021

Fix Windows ninja builds when MAX_JOBS is specified #65444

Closed

malfet mentioned this pull request Oct 5, 2021

Fix Windows ninja builds when MAX_JOBS is specified (#65444) #66155

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

delegate parallelism to Ninja when possible #64733

delegate parallelism to Ninja when possible #64733

Uh oh!

dagitses commented Sep 9, 2021 •

edited

Loading

Uh oh!

facebook-github-bot commented Sep 9, 2021 •

edited

Loading

Uh oh!

Uh oh!

dagitses commented Sep 15, 2021

Uh oh!

dagitses commented Sep 16, 2021

Uh oh!

dagitses commented Sep 16, 2021

Uh oh!

dagitses commented Sep 16, 2021

Uh oh!

Uh oh!

dagitses commented Sep 16, 2021

Uh oh!

dagitses commented Sep 17, 2021

Uh oh!

facebook-github-bot commented Sep 17, 2021

Uh oh!

cloudhan commented Sep 22, 2021 •

edited

Loading

Uh oh!

dagitses commented Sep 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

delegate parallelism to Ninja when possible #64733

delegate parallelism to Ninja when possible #64733

Uh oh!

Conversation

dagitses commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

💊 CI failures summary and remediations

Uh oh!

Uh oh!

dagitses commented Sep 15, 2021

Uh oh!

dagitses commented Sep 16, 2021

Uh oh!

dagitses commented Sep 16, 2021

Uh oh!

dagitses commented Sep 16, 2021

Uh oh!

Uh oh!

dagitses commented Sep 16, 2021

Uh oh!

dagitses commented Sep 17, 2021

Uh oh!

facebook-github-bot commented Sep 17, 2021

Uh oh!

cloudhan commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dagitses commented Sep 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dagitses commented Sep 9, 2021 •

edited

Loading

facebook-github-bot commented Sep 9, 2021 •

edited

Loading

cloudhan commented Sep 22, 2021 •

edited

Loading