Add support for controlling the number of jobs used by some cmake tasks by KazNX · Pull Request #107 · colcon/colcon-cmake

KazNX · 2021-07-26T23:02:48Z

This change adds support for build arg --cmake-jobs to set the number of jobs to use to preserve CPU for the OS. It also renames _get_make_arguments() to _get_make_jobs_arguments() to better reflect the responsibilities of the function.

There is currently support to set the number of job threads to the maximum available, unless the MAKEFLAGS environment variable is set. The assumption is that MAKEFLAGS will be used by the build framework to control the number of jobs. However, this is only true for make - as far as I can tell, ninja does not have any such environment variable based control (related issues reported against ninja 922, 1482).

When MAKEFLAGS is not set, colcon will maxout the -j argument for ninja. Since ninja is so efficient, this can make the computer unresponsive for the OS and other applications. This change has been made to allow the OS to remain somewhat responsive.

As part of the change, I noted that _get_make_arguments() appears to be misnamed as the only thing it does is set the number of jobs to use, as evidenced by the use of the returned values. I've renamed it _get_make_jobs_arguments() to better reflect this. Happy to revert this change if required.

The --cmake-jobs args supports the following usage patterns:

Positive number - use that many threads.
Zero - use all available threads (current behaviour)
Negative - use all but the absolute value of this number of threads (minimum use 1 thread).

- Add support for `build` arg `--cmake-jobs` to set the number of jobs to use. - Rename `_get_make_arguments()` to `_get_make_jobs_arguments()` for clarity

KazNX · 2021-08-30T03:45:27Z

PR has been rebased onto master fixing the CI check failures.

hidmic

Overall this LGTM. @dirk-thomas ?

Slight reorganisation to change jobs variable initialisation. Co-authored-by: Michel Hidalgo <michel@ekumenlabs.com>

Rename `is_job_base_generator` to `is_job_based_generator` to be more correct

This better reflects the nature of the function.

hidmic

This looks great to me, but I'd appreciate @dirk-thomas or @cottsay input as well.

cottsay

Doesn't simply specifying the CMAKE_BUILD_PARALLEL_LEVEL environment variable work for Ninja?

cottsay · 2021-09-14T22:49:07Z

+            type=int,
+            help='Number of jobs to use for supported generators (e.g., Ninja '
+            'Makefiles). Negative values subtract from the maximum '
+            'available, so --jobs=-1 uses all bar 1 available threads.')


Suggested change

'available, so --jobs=-1 uses all bar 1 available threads.')

'available, so --jobs=-1 uses all but 1 available threads.')

Co-authored-by: Michel Hidalgo <michel@ekumenlabs.com>

KazNX

Doesn't simply specifying the CMAKE_BUILD_PARALLEL_LEVEL environment variable work for Ninja?

@cottsay Not sure, but it was introduced in CMake 3.12. I'm working with baseline support for 3.10 as ships with Ubuntu 18.04.

KazNX · 2021-07-26T23:04:47Z

+    :rtype: bool
+    """
+    known_jobs_base_multi_configuration_generators = (
+        'Ninja Multi-Config',


I originally made this change on top of PR 106 then pulled this out. You can view this as a defensive or pre-emptive change. It should have little to no impact until then as the ninja multi-config won't really work yet.

KazNX · 2021-07-26T23:05:52Z

                cmd += ['--config', self._get_configuration(args)]
-            else:
-                job_args = self._get_make_arguments(env)
+            if jobs_base_generator:


Similarly, the removal of the else and replacing it with this separate condition check relates to ninja multi-config support, but should have no impact in isolation.

KazNX · 2021-07-26T23:06:22Z

+            # Finalise jobs as as CPU count deducting the limit specified.
+            jobs = max(jobs + jobs_args, 1)
        return [
            '-j{jobs}'.format_map(locals()),


The rename of the function is based on this result. It can only return jobs. Will revert if appropriate.

KazNX · 2021-07-26T23:06:50Z

            cmd += ['--config', self._get_configuration(args)]
-        elif allow_job_args:
-            job_args = self._get_make_arguments(env)
+        if allow_job_args:


This elif to if change also supports ninja multi-config.

KazNX · 2021-09-15T03:23:46Z

+            type=int,
+            help='Number of jobs to use for supported generators (e.g., Ninja '
+            'Makefiles). Negative values subtract from the maximum '
+            'available, so --jobs=-1 uses all bar 1 available threads.')


KazNX · 2021-09-15T22:11:55Z

@cottsay Looking at the CMake 3.12 release notes, it looks like they added --parallel and --jobs arguments as well as CMAKE_BUILD_PARALLEL_LEVEL. This is great, but I need to target a minimum version of CMake 3.10 for now (the default CMake version for Ubuntu 18.04).

This changes the context of this PR a bit and I see the need to revise the original code I modified to deal with that as well. I propose the ideal solution is to use the changes I've made with CMake < 3.12. For CMake 3.12+, change this function to return --parallel and --jobs instead, which special provisions not to append these after -- is added to the command line. _is_jobs_based_generator() can then return True in call cases.

My question is, are the changes better made in this PR or a to push this through and start a new PR?

cottsay · 2021-09-15T22:56:45Z

are the changes better made in this PR or a to push this through and start a new PR?

I see two conceptual changes:

We should to stop mucking with the MAKEFLAGS if CMAKE_BUILD_PARALLEL_LEVEL is specified in the environment, which would support this use case for any system with CMake 3.12 or newer. This should be a trivial change. We should not remove the existing MAKEFLAGS arguments outright in favor of CMAKE_BUILD_PARALLEL_LEVEL because it uses --load-average, which achieves different behavior than --jobs alone.
We should introduce a mechanism to support your use case with CMake older than 3.12. I'm not sure if a full-fledged command line option is the right move or something a little more "hidden" like an environment variable. Maybe a deprecated command line option would be good - or one that shows a deprecation warning if you're already using 3.12 and should switch to the supported environment variable for doing this.

The latter can be done in this PR, I think. The former should probably be done in a new PR. Thoughts?

KazNX · 2021-09-15T23:56:43Z

I see two conceptual changes:

I fully agree with point 1, but would add that it's not just using CMAKE_BUILD_PARALLEL_LEVEL. It needs to be either/or with passing cmake args for --parallel and/or --jobs. I forsee some annoying permutations and need to learn more about how these interact.

On point 2, the deprecation warning only makes sense if CMAKE_BUILD_PARALLEL_LEVEL or --parallel and/or --jobs are present. Something like:

if cmake_ver >= 3.12 and (have_env_var('CMAKE_BUILD_PARALLEL_LEVEL') or have_cmake_arg('--parallel') or have_cmake_arg('--jobs')):
  print('deprecation warning')

Again, I need more information aboug how the command line arguments interact with CMAKE_BUILD_PARALLEL_LEVEL. Will get back to you on that.

KazNX · 2021-09-16T00:38:51Z

FYI, -j --parallel and CMAKE_BUILD_PARALLEL_LEVEL are all the same. Command line help from CMake 3.12;

-j [] --parallel [] = Build in parallel using
the given number of jobs. If is omitted
the native build tool's default number is used.
The CMAKE_BUILD_PARALLEL_LEVEL environment variable
specifies a default parallel level when this option
is not given.

Revised how jobs are specified. For cmake 3.12+, we use `cmake --build -jN`. For cmake < 3.12, we use `cmake --build [args] -- -jN -lN`, but only for Ninja and Makefiles generators. Note we no longer check if we have a jobs based generator. This is now managed internally to `_get_jobs_arguments()`. Also note we now add `--` inside `_get_jobs_arguments()` as it is not needed for CMake 3.12+

KazNX · 2021-09-17T02:04:41Z

@cottsay @hidmic
I've made a significant revision of this PR based on CMake 3.12+ supporting --parallel or -jN. Note that it sill uses a new colcon build argument --cmake-jobs in all cases. I can't use --cmake-args because that only affects CMake configuration and we need to inject arguments into cmake --build.

The changes are summaried as:

For cmake 3.12+, we use cmake --build -jN.
For cmake < 3.12, we use cmake --build [args] -- -jN -lN, but only for Ninja and Makefiles generators.

Note we no longer check if we have a jobs based generator. This is now managed internally to _get_jobs_arguments().

Also note we now add -- inside _get_jobs_arguments() as it is not needed for CMake 3.12+

dirk-thomas · 2021-09-17T05:07:42Z

Since I don't have the spare time to review this change I leave it up to the co-maintainers to review and test this.

A general - fairly significant - concern I have: the Travis CI integration for all colcon repositories has been broken for a while. Merging any changes has an increased risk to introduce regressions. I strongly suggest to first "get the house in order" and reestablish CI coverage for the lost platforms - e.g. by adding GitHub action support to all repos.

KazNX · 2021-09-27T22:37:04Z

@cottsay @hidmic Correct me if I'm wrong, but I read Dirk's comments as concerns over the current state of master rather than a applying to this PR. In that light, are there any further changes required to get this PR merged?

cottsay · 2021-09-27T22:53:51Z

...over the current state of master rather than a applying to this PR.

Neither this PR nor master are getting test coverage on Linux or macOS because of the missing Travis builds.

are there any further changes required to get this PR merged?

I'm still not feeling great about merging a PR that adds a command line option to support such a narrow use case, especially one that has an official mechanism available in newer versions of CMake.

Here's a completely different suggestion - could we expose a free-form --cmake-build-args option to correspond to the existing free-form --cmake-args option? That would be useful outside of the Ninja use case, and wouldn't be obsoleted by CMake 3.12+.

KazNX · 2021-09-27T23:18:22Z

Here's a completely different suggestion - could we expose a free-form --cmake-build-args option to correspond to the existing free-form --cmake-args option? That would be useful outside of the Ninja use case, and wouldn't be obsoleted by CMake 3.12+.

Yes, I like this option. It does mean removing the _get_jobs_arguments() method (previously _get_make_arguments()) completely because to prevent it from adding implied -j arguments.

KazNX · 2021-09-28T00:36:17Z

I've created a new PR for the --cmake-build-args approach. See here #110

I managed to maintain _get_make_arguments(), though I think that functionality is questionable.

We can deprecate this PR if the 110 is the preferred solution.

Kazys Stepanas added 2 commits August 30, 2021 13:25

Add support for controlling the number of jobs used by some cmake tasks

3848b2a

- Add support for `build` arg `--cmake-jobs` to set the number of jobs to use. - Rename `_get_make_arguments()` to `_get_make_jobs_arguments()` for clarity

Fix use of wrong quotes

3b8132e

KazNX force-pushed the feature/build-jobs branch from d1b7eb0 to 3b8132e Compare August 30, 2021 03:25

American spelling fix

9bcbaec

hidmic reviewed Sep 10, 2021

View reviewed changes

Comment thread colcon_cmake/task/cmake/build.py Outdated

Fix handling of positive job numbers.

c726514

hidmic reviewed Sep 13, 2021

View reviewed changes

Comment thread colcon_cmake/task/cmake/build.py Outdated

Comment thread colcon_cmake/task/cmake/build.py Outdated

Comment thread colcon_cmake/task/cmake/__init__.py Outdated

KazNX and others added 3 commits September 14, 2021 09:29

Revised use of cmake_jobs args

9fc9315

Slight reorganisation to change jobs variable initialisation. Co-authored-by: Michel Hidalgo <michel@ekumenlabs.com>

Function rename

4bb784c

Rename `is_job_base_generator` to `is_job_based_generator` to be more correct

Rename _get_make_jobs_arguments to _get_jobs_arguments

9afca3b

This better reflects the nature of the function.

hidmic approved these changes Sep 14, 2021

View reviewed changes

Comment thread colcon_cmake/task/cmake/build.py Outdated

cottsay reviewed Sep 14, 2021

View reviewed changes

Update colcon_cmake/task/cmake/build.py

2b936c2

Co-authored-by: Michel Hidalgo <michel@ekumenlabs.com>

KazNX commented Sep 15, 2021

View reviewed changes

Fix comment exceeding line length

3c2ba6f

KazNX mentioned this pull request Sep 28, 2021

Args options for CMake --build and --install #110

Open

	'available, so --jobs=-1 uses all bar 1 available threads.')
	'available, so --jobs=-1 uses all but 1 available threads.')

Uh oh!

Conversation

KazNX commented Jul 26, 2021

Uh oh!

KazNX commented Aug 30, 2021

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hidmic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cottsay left a comment

Choose a reason for hiding this comment

Uh oh!

cottsay Sep 14, 2021

Choose a reason for hiding this comment

Uh oh!

KazNX Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

KazNX left a comment

Choose a reason for hiding this comment

Uh oh!

KazNX Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

KazNX Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

KazNX Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

KazNX Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KazNX Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

KazNX commented Sep 15, 2021

Uh oh!

cottsay commented Sep 15, 2021

Uh oh!

KazNX commented Sep 15, 2021

Uh oh!

KazNX commented Sep 16, 2021

Uh oh!

KazNX commented Sep 17, 2021

Uh oh!

dirk-thomas commented Sep 17, 2021

Uh oh!

KazNX commented Sep 27, 2021

Uh oh!

cottsay commented Sep 27, 2021

Uh oh!

KazNX commented Sep 27, 2021

Uh oh!

KazNX commented Sep 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants