Skip to content
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

Question | Could mixin/extensions be used for supporting incremental build and test? #213

Closed
ruffsl opened this issue Aug 23, 2019 · 10 comments
Labels
question Further information is requested

Comments

@ruffsl
Copy link
Member

ruffsl commented Aug 23, 2019

Say a user has built a workspace and subsequently runs colcon test. The user then modifies a number of files in the workspace and again invokes colcon build and then colcon test. Ideally, one could expect packages that are were modified, as well packages above those modified, to be rebuilt. Then for testing, only those packages rebuilt, would be retested. Thus the caching of build and test would be maximized, yet still encompassing all changes to the workspace since the previous build and test.

Would this be achievable using mixins and extensions? Or would core changes be required?

I'm thinking an extension could retain a digest-manifest for all files in the workspace, so it could be fully aware of file level changes. Those file changes could then be associated to package names that then could be passed to colcon build and test arg, e.g.

colcon build --mixin incremental
->> expands to ->>
colcon build --packages-above <modified_packages> \
    --packages-select-build-failed
colcon test --mixin incremental
->> expands to ->>
colcon test --packages-above <rebuilt_packages> \
    --packages-select-test-failures

BTW, are the sets of packages captured by each arg OR'd or AND'd together?
And would something like incremental clean vs incremental dirty be helpful for specifying whether effected packages should be cleaned or not prior to build or test? Related: #37

@dirk-thomas dirk-thomas added the question Further information is requested label Aug 24, 2019
@dirk-thomas
Copy link
Member

Ideally, one could expect packages that are were modified, as well packages above those modified, to be rebuilt.

I don't think that you can make this assumption. Maybe the user does want to build and test all packages. So first of all this needs an explicit statement that this is what the user would like to do imo.

I'm thinking an extension could retain a digest-manifest for all files in the workspace, so it could be fully aware of file level changes.

Your assumption that a package only needs to be rebuild if any files within the package itself change isn't correct. There could be changes in the environment or dependencies used by the package like header files on the system which still required the package to be rebuild. Again I think in this case an explicit statement of intend is necessary that this is what the user wants to do and is aware of the limitations.

Would this be achievable using mixins and extensions?

A mixin doesn't have any logic. A mixin name is simply expanded into a set of command line arguments. So it can't determine the package names needed in the current situation (based on whatever state). This kind of functionality needs to be in an extension with Python code to implement the logic.

are the sets of packages captured by each arg OR'd or AND'd together?

The existing --packages-* options all apply an AND logic so all conditions have to match in order for a package to be processed. (You will only find selected = False assignment in colcon-package-selection, never a selected = True.)

--packages-above <modified_packages>

The question here is "what-means-modified"? Any file under the package directory (how about .git subdirectories`)? That state information would need to be persisted during previous build invocations.

--packages-above <rebuilt_packages>

The question here is "rebuilt-since-when"? The last build invocation? That state information would need to be persisted during the build.

The following question applies to both cases: would it be better to add a new option for this (like --packages-above-last-built) or instead add a concept of "keywords" which can be passed to the existing --packages-* options which know how to "expand" such keywords. The former is certainly easier and more modular - the latter would require all existing options to play along and be aware of the keywords which is pretty cross cutting.

would something like incremental clean vs incremental dirty be helpful for specifying whether effected packages should be cleaned or not prior to build or test?

I don't understand the question - maybe you can elaborate on it.

@ruffsl
Copy link
Member Author

ruffsl commented Aug 24, 2019

So first of all this needs an explicit statement that this is what the user would like to do imo.

Agreed, this incremental behavior should be explicitly specified and opt in. I was thinking that providing a mixin would serve as the explicit statement, but as you pointed out: mixin name simply expand to sets of command line arguments. Can an extension add sub arguments to the command line arg parse, or can they only add behaviors to existing args? Do any of the common extensions demonstrate this?

https://github.com/colcon/colcon-common-extensions/blob/master/stdeb.cfg#L3

There could be changes in the environment or dependencies used by the package like header files on the system which still required the package to be rebuild. Again I think in this case an explicit statement of intend is necessary that this is what the user wants to do and is aware of the limitations.

Yes, changes in the system could necessitate a fresh rebuild. The eventual use case I had in mind was for a speedy local CI agent in a container that could use the container image as a cache-break event, thus if the same docker image binary was used for the build step, then the same build artifacts could be reused. Here is an example of where we use a chaining checksum as the key to cache workspaces, seeded using the last touched file in the docker image build and system dependencies installed:

Aside from

The existing --packages-* options all apply an AND logic so all conditions have to match in order for a package to be processed. (You will only find selected = False assignment in colcon-package-selection, never a selected = True.)

Excellent, that's a good detail to know. Is that documented anywhere on the docs?
I didn't spot this in the select package sections.

https://github.com/colcon/colcon.readthedocs.org/blob/f827f8a/user/how-to.rst

The question here is "what-means-modified"? Any file under the package directory (how about .git subdirectories`)? That state information would need to be persisted during previous build invocations.

Yes, the extension would have to store a checksum or digest manifest for each package to remember the state of the source directory. The extension could also make use of the .*ignore files in the source folder to prevent IDE and vcs(git) meta files from breaking caches.

The question here is "rebuilt-since-when"? The last build invocation? That state information would need to be persisted during the build.

Would the respective package subdirectories in the build folder be a good place to store this state information? Like where the return codes for build and test command for each package are kept.

The former is certainly easier and more modular - the latter would require all existing options to play along and be aware of the keywords which is pretty cross cutting.

I guess I'd lean towards the former. As long as it would play nicely with colcon list then users could make queries about package names that they could then merge or split however they want with the rest of the existing commands. E.g. here is how we split the colcon tests across parallel workers by weighting the package split using historic test timing results.

https://github.com/ros-planning/navigation2/blob/c1b4fe4bfecd7a3a374119b7190870112510b271/.circleci/config.yml#L146-L150

I don't understand the question - maybe you can elaborate on it.

If we can entirely account for the state of the system outside the workspace (e.g. detect with the workspace and system fall out of sync given changes to the system), how necessary is it to rm -rf or clean the build folder. I recall encountering something similar to this #37 (comment) if I wasn't diligent in cleaning my workspace when I made changes in my source folder.

If I were to rm -rf just the install folder, could one rely on the incremental build strategy of the build system underneath, like CMake, to keep the install up-to-date while retaining build speed? I suspect stale artifacts could still linger in the build folder, even if they no longer propagate into the install folder. This might make it difficult to cache the entire build directory.

Perhaps I'm over thinking things, but if IIRC, colcon build left unspecified just enumerates all packages in topological order. For building all of ros2 in a single workspace, I think even waiting on the incremental build to finish can take some time, thus I suppose some of the motivation for overlaying workspaces and arguments like --packages-start. Do you think something like --packages-above-last-built would be an appropriate response, or is this just something the build system should seek to do better?

@dirk-thomas
Copy link
Member

Can an extension add sub arguments to the command line arg parse, or can they only add behaviors to existing args? Do any of the common extensions demonstrate this?

Several of the extension points allow an extension to contribute to the parser. Just search for .add_argument( in any of the colcon packages.

The existing --packages-* options all apply an AND logic ...

Excellent, that's a good detail to know. Is that documented anywhere on the docs?

The current docs are focused on example what a user might want to do and atm only contain cases with a single --packages- argument. Adding a case which makes use of multiple different arguments would be good to mention this detail. Though this is a per-argument decision so it could be different for some options.

Would the respective package subdirectories in the build folder be a good place to store this state information? Like where the return codes for build and test command for each package are kept.

Yes, I think that would be the right location.

As long as it would play nicely with colcon list then users could make queries about package names that they could then merge or split however they want with the rest of the existing commands.

Yes, for more complex sets of packages it is possible (and intended, e.g. you can use that to easily perform the OR logic) that you can use colcon list to determine the argument for another colcon * --packages-* call. One example in the docs: https://github.com/colcon/colcon.readthedocs.org/blob/f827f8a5761d8e526aab191ed9a010552d309f53/user/how-to.rst#test-selected-packages-as-well-as-their-dependents

If I were to rm -rf just the install folder, could one rely on the incremental build strategy of the build system underneath, like CMake, to keep the install up-to-date while retaining build speed?

Well, it depends 😉 usually you should be fine but there are corner cases. E.g. a left over artifact in the build directory could have an undesired effect or a downstream package has cached information in its CMakeCache.txt which aren't up-to-date with the updated install of an upstream package. So for 100% correct behavior you will need to delete build too and instead rely on something like ccache to still achieve a speedy rebuild.

Do you think something like --packages-above-last-built would be an appropriate response, or is this just something the build system should seek to do better?

Usually a full build will take less than a second per previously build and unchanged package. If that is fast enough is certainly subjective. If you have only done a single build of a subset of packages before the option --packages-skip-build-finished should give you exactly that: build all packages which haven't been built before (or failed).

@ruffsl
Copy link
Member Author

ruffsl commented Aug 24, 2019

So for 100% correct behavior you will need to delete build too

When re-building packages per --packages-above, considering topological order and no system disturbances, would deleting the build/<package_name>/ folder for above packages afford equivalent certainty in correct behavior, just as if we deleted the entire build folder? This kind of partial workspace reuse could be a middle ground for those who'd prefer the speed from incremental build in a 'dirty' workspace, but would also like a correct build from a clean workspace.
E.g. --packages-skip-source-unchanged.

I'm guessing removing packages from the install folder would not be as straightforward, as if the source changes, we might not have the same uninstall behavior (is that a even a thing here). Thus the conservative route would be to wipe the install folder entirely, and call build for all packages to recreate the install directory, with a clean ament index and no dangling links. However, I suppose this would limit the use of --packages-skip-source-unchanged, or is this not so much an issue or corner case?

and instead rely on something like ccache to still achieve a speedy rebuild.

Yep, we're already using ccache to speed up the CI, as we pre-heat the ccache from the nightly docker image, and keep it warm by limiting the cache's scope to the particular PR so hits for the branch are less likely to age out ~/.ccache.

https://github.com/ros-planning/navigation2/blob/c1b4fe4bfecd7a3a374119b7190870112510b271/.circleci/config.yml#L286-L297

BTW, if you know what non-determinism might be causing the miserable hit rates for the debug build, as opposed to the 100% rate we have for release builds, let me know what you might think:
https://answers.ros.org/question/331493/poor-ccache-hit-rate-on-debug-build-with-coverage-set-using-colcon/

@dirk-thomas
Copy link
Member

When re-building packages per --packages-above, considering topological order and no system disturbances, would deleting the build/<package_name>/ folder for above packages afford equivalent certainty in correct behavior, just as if we deleted the entire build folder?

Yes, deleting the build directories of all packages about to be processed will result in correct behavior.

I'm guessing removing packages from the install folder would not be as straightforward, as if the source changes, we might not have the same uninstall behavior (is that a even a thing here).

You uninstall steps shouldn't be based on the current state of the source directory (since as you mentioned that might have changed). The necessary steps to uninstall the package need to be generated at build time and when triggering an uninstall the information from the previous build must be used. (That might still be incomplete since the sources might have changed in the past without ever triggering an uninstall.)

Thus the conservative route would be to wipe the install folder entirely, and call build for all packages to recreate the install directory, with a clean ament index and no dangling links.

If you use an isolated install (which is the default when building from source) you can simply delete the package specific install directories install/<package_name> (rather than wiping the whole install space).

@ruffsl
Copy link
Member Author

ruffsl commented Aug 26, 2019

Yes, deleting the build directories of all packages about to be processed will result in correct behavior.

Awsome!

If you use an isolated install (which is the default when building from source) you can simply delete the package specific install directories install/<package_name> (rather than wiping the whole install space).

Ok, so assuming an isolated install, and the hypothetical --packages-skip-source-unchanged, would the following use patterns make sense?

  • Cache builds from unchanged packages
  • Re-test only effected packages
  • via:
PKGS_CHANGED=$(colcon list --packages-skip-source-unchanged | xarg)
PKGS_DEP=$(colcon list --packages-select-by-dep $PKGS_CHANGED | xarg)

Minimal caching but Correct build

Wipe both build and install spaces for changed and dependent packages

colcon clean --build --install --packages-select $PKGS_CHANGED $PKGS_DEP
colcon build --packages-select $PKGS_CHANGED $PKGS_DEP
colcon test --packages-select $PKGS_CHANGED $PKGS_DEP

Moderate caching but Imprecise build

Wipe only install spaces for changed packages
Wipe both build and install spaces for dependent packages
Leverage incremental build from build systems for changed packages

colcon clean --install --packages-select $PKGS_CHANGED
colcon clean --build --install --packages-select $PKGS_DEP
colcon build --packages-select $PKGS_CHANGED $PKGS_DEP
colcon test --packages-select $PKGS_CHANGED $PKGS_DEP

Maximum caching but Imprecise build

Wipe only install spaces for changed and dependent packages
Leverage incremental build from build systems for changed and dependent packages

colcon clean --install --packages-select $PKGS_CHANGED $PKGS_DEP
colcon build --packages-select $PKGS_CHANGED $PKGS_DEP
colcon test --packages-select $PKGS_CHANGED $PKGS_DEP

@dirk-thomas
Copy link
Member

so assuming an isolated install, and the hypothetical --packages-skip-source-unchanged, would the following use patterns make sense?

  • Minimal caching but Correct build
  • Moderate caching but Imprecise build
  • Maximum caching but Imprecise build

Yes, these processes should be fine.

For an extra safe guard (in the last case where you didn't wipe build) you could use --cmake-clean-cache on the build invocation to ensure that at least the CMake cache isn't using outdated information.

--packages-select $PKGS_CHANGED $PKGS_DEP

You should be able to use --packages-above $PKGS_CHANGED for this without the need to additionally determine the dependencies explicitly.

colcon clean

You want to watch out for the exact verb / term here. In #37 where such a functionality is discussed there is some controversy about the term to be used: clean / delete / wipe (since clean has a different semantic in the used build systems like CMake / make).

@ruffsl
Copy link
Member Author

ruffsl commented Aug 26, 2019

For an extra safe guard (in the last case where you didn't wipe build) you could use --cmake-clean-cache on the build invocation to ensure that at least the CMake cache isn't using outdated information.

Would using --cmake-clean-cache result in more calls to make/ccache for objects still present in the build folder, or would make/ccache still only be invoked for source files truly affected by changes?

https://github.com/colcon/colcon-cmake/blob/7d5bbeafc2e52b979c93e5c0215d805744eaf3c1/colcon_cmake/task/cmake/build.py#L124-L127

You should be able to use --packages-above $PKGS_CHANGED for this without the need to additionally determine the dependencies explicitly.

Here is a DRY'er mock up using in-place args to avoid tracking around two variables:

PKGS_CHANGED=$(colcon list --packages-skip-source-unchanged | xarg)

Minimal caching but Correct build

colcon clean --build --install --packages-above $PKGS_CHANGED
colcon build --packages-above $PKGS_CHANGED --cmake-clean-cache
colcon test  --packages-above $PKGS_CHANGED

Moderate caching but Imprecise build

colcon clean --install --packages-select $PKGS_CHANGED
colcon clean --build --install --packages-select-by-dep $PKGS_CHANGED
colcon build --packages-above $PKGS_CHANGED --cmake-clean-cache
colcon test  --packages-above $PKGS_CHANGED

Maximum caching but Imprecise build

colcon clean --install --packages-above $PKGS_CHANGED
colcon build --packages-above $PKGS_CHANGED --cmake-clean-cache
colcon test  --packages-above $PKGS_CHANGED

@dirk-thomas
Copy link
Member

Would using --cmake-clean-cache result in more calls to make/ccache for objects still present in the build folder, or would make/ccache still only be invoked for source files truly affected by changes?

It will only result in a full CMake configure run. The actual build doesn't change if the source files / all included headers didn't change.

@dirk-thomas
Copy link
Member

@ruffsl If the question has been answered can you go ahead and close the ticket?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Development

No branches or pull requests

2 participants