Add new initial command stage to mutex all reads of the installspace. #391

mikepurvis · 2016-07-23T03:03:40Z

This is the solution to #378 proposed in #378 (comment).

There's probably a small perf hit here which could be mitigated by going to a readers-writer lock, but it doesn't seem Python has one natively (example implementation), and the cost of switching the locked_resource scheme to support such a thing would increase complexity. Given how fast the loadenv stage is relative to the rest of the build, I don't feel it's worth it.

@jbohren @wjwwood

My validation methodology for this is as described in #378 (comment), with one small change— I've duplicated the test workspace and am running two instances of it in parallel, in the hopes of increasing overall entropy on the box and triggering problems.

As of July 24th, I've built ros_base ~2000 times across the two workspaces, with no failed builds. Previous tests with catkin_tools 0.4.2 get failures of the ros_base workspace every 100-200 builds (we see them much more frequently than this on our bundle builds, but those are much larger and more heavily parallelized than the ros_base workspace is).

Upon switching back to the master branch (4f5fa9d), I got the following failures in my two test ros_base workspaces. The error outputs below are manifestations of the underlying concurrency problem which this PR corrects.

Run 11 in the first workspace:

Errors     << shape_msgs:cmake /home/mpurvis/catkin_tools_fail/logs/shape_msgs/build.cmake.000.log
CMake Warning at /opt/clearpath/2.1devel/sdk/share/catkin/cmake/catkinConfig.cmake:76 (find_package):
  Could not find a package configuration file provided by "geometry_msgs"
  with any of the following names:

    geometry_msgsConfig.cmake
    geometry_msgs-config.cmake

  Add the installation prefix of "geometry_msgs" to CMAKE_PREFIX_PATH or set
  "geometry_msgs_DIR" to a directory containing one of the above files.  If
  "geometry_msgs" provides a separate development package or SDK, be sure it
  has been installed.

Run 107 in the second workspace:

Errors     << sensor_msgs:cmake /home/mpurvis/catkin_tools_fail2/logs/sensor_msgs/build.cmake.000.log
Traceback (most recent call last):
  File "/home/mpurvis/catkin_tools_fail2/build/sensor_msgs/catkin_generated/generate_cached_setup.py", line 20, in <module>
    from catkin.environment_cache import generate_environment_script
ImportError: No module named catkin.environment_cache

Next failure on the 22nd run:

/usr/bin/python: can't open file '/home/mpurvis/catkin_tools_fail2/install/_setup_util.py': [Errno 2] No such file or directory
Finished  <<< mk                                   [ 2.0 seconds ]
Starting  >>> geometry_msgs
Starting  >>> rosgraph_msgs
______________________________________________________________________________________
Errors     << diagnostic_msgs:cmake /home/mpurvis/catkin_tools_fail2/logs/diagnostic_msgs/build.cmake.000.log
Traceback (most recent call last):
  File "/home/mpurvis/catkin_tools_fail2/build/diagnostic_msgs/catkin_generated/generate_cached_setup.py", line 20, in <module>
    from catkin.environment_cache import generate_environment_script
ImportError: No module named catkin.environment_cache

Then run 59:

Starting  >>> sensor_msgs
Starting  >>> shape_msgs
Starting  >>> trajectory_msgs
Starting  >>> visualization_msgs
sh: 0: Can't open /home/mpurvis/catkin_tools_fail/install/env.sh
WARNING: Sourced environment from `/home/mpurvis/catkin_tools_fail/install/env.sh` has no environment variables. Something is wrong.
Finished  <<< rosconsole                           [ 8.7 seconds ]
Starting  >>> pluginlib
Starting  >>> rosconsole_bridge
Starting  >>> roscpp
Finished  <<< roslisp                              [ 1.8 seconds ]
______________________________________________________________________________________
Errors     << trajectory_msgs:cmake /home/mpurvis/catkin_tools_fail/logs/trajectory_msgs/build.cmake.000.log
CMake Warning at /opt/clearpath/2.1devel/sdk/share/catkin/cmake/catkinConfig.cmake:76 (find_package):
  Could not find a package configuration file provided by
  "message_generation" with any of the following names:

    message_generationConfig.cmake
    message_generation-config.cmake

mikepurvis · 2016-07-23T15:41:56Z

catkin_tools/jobs/utils.py

+
+    for env_loader_path in env_loader_paths:
+        if logger:
+            logger.out('Loading environment from: {}'.format(env_loader_path))


The logger.out could just be dropped if we don't like it; the conditional is because of the print_build_env invocation, which doesn't have a logger to pass it.

wjwwood · 2016-07-25T18:06:40Z

The code changes and approach seem ok to me, though I share your concern about the performance impact. I wonder how the impact on performance changes as the workspace size increases.

I'd like a once over by @jbohren before merging this though, as he's more familiar with the env loading code.

mikepurvis · 2016-07-25T18:56:52Z

The perf impact will be proportional to parallelism rather than number of packages, I think, since the impact will be having a bunch of packages which all need to start at once having to take turns using the installspace to set up their environment.

That said, if there's any measurable impact as a result of this, it's us you can expect a fix from. :)

Edit to add: Would like to get this merged and released as soon as possible, but as of this morning we're running it from source in production— will report back if there are any anomalies with it.

mikepurvis · 2016-07-29T17:59:54Z

We've been running this for the last couple days for our bundle builds, and the spurious failures have completely disappeared. I have a report from a ros-install-osx user who hit the problem on a multi-core Mac build, so I am keen to see this merged and released.

I'd appreciate input particularly on the business of running the env setup for create_catkin_clean_job and create_cmake_clean_job. My instinct is that it shouldn't ever be necessary, given that those jobs are mostly composed of FunctionStage instances which don't use the environment anyway, and the only CommandStage is to run make clean, which only uses the cwd.

@jbohren Have you had a chance to look at this at all?

This better aligns the function name with other similar helpers in catkin_tools.jobs.utils.

mikepurvis · 2016-08-10T20:28:05Z

@wjwwood @jbohren Would be delighted for you to look at this soon so that we can get this merged and released— it does resolve an actual bug in the wild which affects large parallel builds.

I have now renamed the new function to loadenv, made the FunctionStage name match, and also dropped this stage from clean jobs. I've tested with building, partially and fully cleaning, and re-building workspaces using this change, and have had no problems. This simplifies the overall diff and avoids some superstitious functionality.

mikepurvis · 2016-08-16T23:53:43Z

Small fix at the behest of flake8; got a clean build now.

mikepurvis · 2016-08-30T13:44:27Z

ping

NikolausDemmel · 2016-08-30T21:33:56Z

I haven't looked at the implementation in detail, but I have been running with this patch on my dev machine for two weeks without any issues.

So +1 from me.

mikepurvis · 2016-09-08T16:53:18Z

ping

mikepurvis · 2016-09-27T16:03:48Z

ping

wjwwood · 2016-09-28T08:50:43Z

I'm sorry @mikepurvis, I'm not likely to have time to review and merge this until after ROSCon. I'll do my best to have a look at it soon. Also, I don't think @jbohren ever got around to skimming it.

mikepurvis · 2016-10-24T18:59:35Z

Just wanting to make sure it doesn't fall off the radar altogether. It's pretty inconvenient for us not having these changes in the native python-catkin-tools deb package (we have to pip install it from the master branch tarball inside debian/rules), so I'm definitely keen to see this merged and released at some point.

I could just release a fork under a different name, or patch in the fix with quilt, but those are their own headaches. And I'd like to continue making contributions upstream, which will become considerably harder to justify if I'm also maintaining a fork.

wjwwood · 2016-11-01T23:45:22Z

Sorry @mikepurvis, I'm trying to balance my time between this, rviz, and robot_model-ish stuff. I've recently spent some time on the others, but I haven't had a slice to spend on catkin tools yet. Hopefully I can do that this week.

We still don't have any feedback from @jbohren on this.

mikepurvis · 2016-11-17T04:15:45Z

For anyone else interested, I've set up a PPA and released a version of python-catkin-tools with this patch applied. You can find it here:

https://launchpad.net/~mikepurvis/+archive/ubuntu/catkin

Process was:

dget http://packages.ros.org/ros/ubuntu/pool/main/p/python-catkin-tools/python-catkin-tools_0.4.2-1.dsc
wget https://github.com/catkin/catkin_tools/pull/391.diff
cd python-catkin-tools-0.4.2
quilt import ../391.diff
dch  # Must remove previous changelog entry so that the original archive is include in .changes file.
dpkg-buildpackage -S
dput ppa:mikepurvis/catkin ../python-catkin-tools_0.4.2-1mpu1_source.changes

mikepurvis · 2017-01-18T18:07:34Z

@wjwwood ping

mikepurvis · 2017-01-18T19:01:12Z

Since 0.4.3 got released (b538dea) without this fix included, I've updated my PPA to a new patch release:

https://launchpad.net/~mikepurvis/+archive/ubuntu/catkin/+packages

wjwwood · 2017-01-28T01:13:29Z

Ok, I've just tested this for a few hours today myself. Given a second code review from me (+1) and @mikepurvis's long term usage of catkin tools with this patch, I think I'm going to merge this now, without feedback from @jbohren.

Sorry it took so long @mikepurvis. I'll try to avoid delays like this in the future. I'd also like to look into sharing ownership with others in the community so things don't get hung up waiting on the maintainers again (currently me and @jbohren have push rights).

I want to try and include some more changes before doing a release, but I will avoid delaying too long before making one.

wjwwood · 2017-02-08T18:36:38Z

Just released 0.4.4.

davetcoleman · 2017-02-14T07:14:32Z

Thanks @mikepurvis !

emielsteerneman · 2018-03-07T10:35:46Z

I am not sure if this is the correct place to ask this, but I still get this error.

-- catkin 0.7.8
Traceback (most recent call last):
    File "/media/emiel/HDD/roboteam/workspace/src/roboteam_input/cmake-build-debug/catkin_generated/generate_cached_setup.py", line 20, in <module>
        from catkin.environment_cache import generate_environment_script
ImportError: No module named catkin.environment_cache
CMake Error at /opt/ros/kinetic/share/catkin/cmake/safe_execute_process.cmake:11 (message):
    execute_process(/usr/bin/python
    "/media/emiel/HDD/roboteam/workspace/src/roboteam_input/cmake-build-debug/catkin_generated/generate_cached_setup.py")
    returned error code 1
Call Stack (most recent call first):
    /opt/ros/kinetic/share/catkin/cmake/all.cmake:186 (safe_execute_process)
    /opt/ros/kinetic/share/catkin/cmake/catkinConfig.cmake:20 (include)
    CMakeLists.txt:10 (find_package)

I am not really at home with CMake, so I have no idea where I would have to start fixing this. Could someone point me in the right direction? Thanks

wjwwood · 2018-03-20T03:32:48Z

@emielsteerneman you might want to make a new issue since this cannot be reopened (as pull requests cannot be opened after merging).

Also, you should include more information, like which version of ROS you're using and what version of catkin tools you're using.

emielsteerneman · 2018-03-21T08:01:57Z

Thanks for your response Wjwwood, now I'll know what to next time I run into such an issue. Fortunately, I managed to fix my current issue. My editor didn't source all the required files, such as setup.bash.

rarestg · 2018-11-13T18:09:42Z

Thanks for your response Wjwwood, now I'll know what to next time I run into such an issue. Fortunately, I managed to fix my current issue. My editor didn't source all the required files, such as setup.bash.

Hi! I'm trying to get Clion working with ROS, so I can step through code, and I'm getting the same error you were getting. Do you remember how/where you changed your editor to source setup.bash and other files? Thanks!

emielsteerneman · 2018-11-14T09:25:48Z

Hi! I'm trying to get Clion working with ROS, so I can step through code, and I'm getting the same error you were getting. Do you remember how/where you changed your editor to source setup.bash and other files? Thanks!

Hey rarestg! I did not modify the editor. The trick is to open CLion via the terminal. It is explained here : https://www.comkieffer.com/clion-ros.html . Good luck!

FransGH · 2019-11-19T01:13:52Z

Old thread but had this error too ("No module named catkin.environment_cache") with CLion and full remote development (clion on mac, target on linux-vm). Solution was to set CMAKE_PREFIX_PATH in CMakeLists.txt:

list(APPEND CMAKE_PREFIX_PATH "/opt/ros/kinetic")
list(APPEND CMAKE_PREFIX_PATH "/opt/ros/kinetic/share/catkin/cmake")
find_package(catkin REQUIRED COMPONENTS
        roscpp
        rospy
        std_msgs
        geometry_msgs
        sensor_msgs
        cv_bridge
        image_transport
        message_generation
)

zhouwuxiong · 2024-03-06T12:29:25Z

@FransGH so crazy,it helps a lot

Add new initial command stage to mutex all reads of the installspace.

e9eb801

mikepurvis force-pushed the installspace-mutexing branch from 7436500 to e9eb801 Compare July 23, 2016 12:08

mikepurvis reviewed Jul 23, 2016
View reviewed changes

mikepurvis mentioned this pull request Jul 25, 2016

Compilation failed - ImportError: No module named catkin.environment_cache mikepurvis/ros-install-osx#53

Closed

Changed load_env -> loadenv.

da43594

This better aligns the function name with other similar helpers in catkin_tools.jobs.utils.

mikepurvis force-pushed the installspace-mutexing branch from b9f72d9 to 9cf85dd Compare August 10, 2016 20:19

Drop job env from clean jobs.

22d2c82

mikepurvis force-pushed the installspace-mutexing branch from 9cf85dd to 22d2c82 Compare August 16, 2016 23:35

Drop unused 'import os'

2aaa8cb

This was referenced Aug 17, 2016

ImportError: No module named catkin.environment_cache ros/catkin#806

Closed

New verb proposal: catkin test #397

Open

mikepurvis mentioned this pull request Nov 3, 2016

Generate documentation for workspace? #381

Closed

mikepurvis mentioned this pull request Dec 14, 2016

Permit extending multiple workspaces #303

Open

wjwwood merged commit 5262dec into catkin:master Jan 28, 2017

mikepurvis mentioned this pull request Feb 9, 2017

ImportError: No module named catkin.environment_cache #378

Closed

mikepurvis mentioned this pull request Apr 5, 2017

execvp: catkin_generated/env_cached.sh: Text file busy #438

Open

dodsonmg mentioned this pull request Jan 27, 2020

ImportError: No module named catkin.environment_cache ros/catkin#1056

Closed

zhouwuxiong mentioned this pull request Mar 6, 2024

compile in docker get error "import Error: No model named catkin" gaoxiang12/slam_in_autonomous_driving#144

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new initial command stage to mutex all reads of the installspace. #391

Add new initial command stage to mutex all reads of the installspace. #391

mikepurvis commented Jul 23, 2016 •

edited by wjwwood

Loading

mikepurvis Jul 23, 2016

wjwwood commented Jul 25, 2016

mikepurvis commented Jul 25, 2016 •

edited

Loading

mikepurvis commented Jul 29, 2016 •

edited

Loading

mikepurvis commented Aug 10, 2016

mikepurvis commented Aug 16, 2016 •

edited

Loading

mikepurvis commented Aug 30, 2016

NikolausDemmel commented Aug 30, 2016

mikepurvis commented Sep 8, 2016

mikepurvis commented Sep 27, 2016

wjwwood commented Sep 28, 2016

mikepurvis commented Oct 24, 2016

wjwwood commented Nov 1, 2016

mikepurvis commented Nov 17, 2016 •

edited

Loading

mikepurvis commented Jan 18, 2017

mikepurvis commented Jan 18, 2017

wjwwood commented Jan 28, 2017

wjwwood commented Feb 8, 2017

davetcoleman commented Feb 14, 2017

emielsteerneman commented Mar 7, 2018

wjwwood commented Mar 20, 2018

emielsteerneman commented Mar 21, 2018

rarestg commented Nov 13, 2018

emielsteerneman commented Nov 14, 2018

FransGH commented Nov 19, 2019 •

edited

Loading

zhouwuxiong commented Mar 6, 2024

Add new initial command stage to mutex all reads of the installspace. #391

Add new initial command stage to mutex all reads of the installspace. #391

Conversation

mikepurvis commented Jul 23, 2016 • edited by wjwwood Loading

mikepurvis Jul 23, 2016

Choose a reason for hiding this comment

wjwwood commented Jul 25, 2016

mikepurvis commented Jul 25, 2016 • edited Loading

mikepurvis commented Jul 29, 2016 • edited Loading

mikepurvis commented Aug 10, 2016

mikepurvis commented Aug 16, 2016 • edited Loading

mikepurvis commented Aug 30, 2016

NikolausDemmel commented Aug 30, 2016

mikepurvis commented Sep 8, 2016

mikepurvis commented Sep 27, 2016

wjwwood commented Sep 28, 2016

mikepurvis commented Oct 24, 2016

wjwwood commented Nov 1, 2016

mikepurvis commented Nov 17, 2016 • edited Loading

mikepurvis commented Jan 18, 2017

mikepurvis commented Jan 18, 2017

wjwwood commented Jan 28, 2017

wjwwood commented Feb 8, 2017

davetcoleman commented Feb 14, 2017

emielsteerneman commented Mar 7, 2018

wjwwood commented Mar 20, 2018

emielsteerneman commented Mar 21, 2018

rarestg commented Nov 13, 2018

emielsteerneman commented Nov 14, 2018

FransGH commented Nov 19, 2019 • edited Loading

zhouwuxiong commented Mar 6, 2024

mikepurvis commented Jul 23, 2016 •

edited by wjwwood

Loading

mikepurvis commented Jul 25, 2016 •

edited

Loading

mikepurvis commented Jul 29, 2016 •

edited

Loading

mikepurvis commented Aug 16, 2016 •

edited

Loading

mikepurvis commented Nov 17, 2016 •

edited

Loading

FransGH commented Nov 19, 2019 •

edited

Loading