-
Notifications
You must be signed in to change notification settings - Fork 95
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
add --skip-pip options #412
base: master
Are you sure you want to change the base?
Conversation
Can you explain a little bit more about your use case? Do you expect people to pip install the dependencies separately? Can you just release python package as a ROS package? Is there a different version of the pip dependencies available from apt? Is there a graceful degradation of the packages that have this dependency if it's not present? Is it a python only package, could you release it via pip? This is certainly the simplest solution, but I wonder if there's a clearer way to declare specific dependencies as optional. Since if we just have this option I suspect that people will just start passing this option if they run into an error due to having a pip dependency and then people will no longer be able to reliably run things that are downloaded and installed. I think it's important that we make sure that things packaged are self contained and work out of the box from the debian packages. Requiring parallel installation from a separate toolchain will lead to many more headaches as versions change going forward. We've run into pip installations getting out of date and breaking systems when users have installed tools via pip. In debs you have the option for version dependencies but that breaks when you cross package managers, and require user action out of band. |
Find I'm expecting
I already had many "headaches" on pip dependencies such as ◉ Kei Okada 2016-11-21 8:16 GMT+09:00 Tully Foote notifications@github.com:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this would work, though I'm dubious about if having unresolved pip dependencies is a good idea. If it doesn't break anything else in the toolchain then it might be fine for expert use cases like you've described.
@tfoote @dirk-thomas what do you guys think?
bloom/generators/common.py
Outdated
# L.99(def resolve_more_for_os(rosdep_key, view, installer, | ||
# os_name, os_version):) in bloom/generators/common.py ? | ||
warning("Key '{0}' resolved to '{1}' with installer '{2}' for os '{3}' '{4}', " | ||
"with 'BLOOM_SKIP_PIP' environment raviable, we intentinally skip this rules" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'variable'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"but with the 'BLOOM_SKIP_PIP' environment variable set, this key is intentionally skipped."
bloom/generators/common.py
Outdated
if 'BLOOM_SKIP_PIP' in environ and environ['BLOOM_SKIP_PIP'] == '1' and inst_key == 'pip': | ||
# force set rule to null for pip, we can escape pip in | ||
# L.99(def resolve_more_for_os(rosdep_key, view, installer, | ||
# os_name, os_version):) in bloom/generators/common.py ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either we can or we cannot, but this comment should be removed before merging.
thanks for comment , updated PR based on your comment.
…--
◉ Kei Okada
2017-01-28 11:03 GMT+09:00 William Woodall <notifications@github.com>:
***@***.**** commented on this pull request.
I guess this would work, though I'm dubious about if having unresolved pip
dependencies is a good idea. If it doesn't break anything else in the
toolchain then it might be fine for expert use cases like you've described.
@tfoote <https://github.com/tfoote> @dirk-thomas
<https://github.com/dirk-thomas> what do you guys think?
------------------------------
In bloom/generators/common.py
<#412 (review)>
:
> @@ -114,6 +116,14 @@ def resolve_more_for_os(rosdep_key, view, installer, os_name, os_version):
os_installers,
default_os_installer)
assert inst_key in os_installers
+ if 'BLOOM_SKIP_PIP' in environ and environ['BLOOM_SKIP_PIP'] == '1' and inst_key == 'pip':
+ # force set rule to null for pip, we can escape pip in
+ # L.99(def resolve_more_for_os(rosdep_key, view, installer,
+ # os_name, os_version):) in bloom/generators/common.py ?
+ warning("Key '{0}' resolved to '{1}' with installer '{2}' for os '{3}' '{4}', "
+ "with 'BLOOM_SKIP_PIP' environment raviable, we intentinally skip this rules"
'variable'
------------------------------
In bloom/generators/common.py
<#412 (review)>
:
> @@ -114,6 +116,14 @@ def resolve_more_for_os(rosdep_key, view, installer, os_name, os_version):
os_installers,
default_os_installer)
assert inst_key in os_installers
+ if 'BLOOM_SKIP_PIP' in environ and environ['BLOOM_SKIP_PIP'] == '1' and inst_key == 'pip':
+ # force set rule to null for pip, we can escape pip in
+ # L.99(def resolve_more_for_os(rosdep_key, view, installer,
+ # os_name, os_version):) in bloom/generators/common.py ?
+ warning("Key '{0}' resolved to '{1}' with installer '{2}' for os '{3}' '{4}', "
+ "with 'BLOOM_SKIP_PIP' environment raviable, we intentinally skip this rules"
"but with the 'BLOOM_SKIP_PIP' environment variable set, this key is
intentionally skipped."
------------------------------
In bloom/generators/common.py
<#412 (review)>
:
> @@ -114,6 +116,14 @@ def resolve_more_for_os(rosdep_key, view, installer, os_name, os_version):
os_installers,
default_os_installer)
assert inst_key in os_installers
+ if 'BLOOM_SKIP_PIP' in environ and environ['BLOOM_SKIP_PIP'] == '1' and inst_key == 'pip':
+ # force set rule to null for pip, we can escape pip in
+ # L.99(def resolve_more_for_os(rosdep_key, view, installer,
+ # os_name, os_version):) in bloom/generators/common.py ?
Either we can or we cannot, but this comment should be removed before
merging.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#412 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAeG3D9JZnQ4m3t13rWXqaC4G5O2C6eOks5rWqHzgaJpZM4K3dL4>
.
|
Thinking about this a little bit more. I'm still pretty strongly against having bloom just drop the pip dependencies. I think that if that's what you want to package then a patch could be added in the release repository. But the debian control file and the package.xml should be consistent. So that if you run rosdep on a binary installed package it will report everything satisfied. The general approach that most users use apt and power users rosdep install things works for most use cases with this method. But breaks the expectations of any regular user who adds one of your packages to their workspace to try it out and then follows standard procedures. They can end up with pip installations they weren't expecting and don't know how to clean up. I would much prefer if you looked into packaging the python library. @asmodehn has added some tooling for pure python packages recently. But you could also create some basic CMake wrappers that call setup.py appropriately. If the python packages are released then all your users will benefit, even the power ones. |
I'll just chime in here that I've been experimenting with building plain python packages as part of a regular catkin workspace using a catkin_tools plugin. It's very alpha at this point (see the issues list), but give it a shot if you like: https://github.com/mikepurvis/catkin_tools_python Building a plain python package in-workspace like this should overlay a pip or deb-installed version of the same, but will otherwise behave the same as if the package is part of the underlying system. If it's a repo you control you can even include your own |
@tfoote @mikeferguson Thanks for comment,
Are you referring https://github.com/asmodehn/catkin_pip ? I'll look into this with https://github.com/mikepurvis/catkin_tools_python. The situation/challenge for me is, how to integrate python developer in to ROS ecosystem, specially we had a lot of deep learning related library written in python (https://pypi.python.org/pypi/pytorch, https://pypi.python.org/pypi/chainer, https://pypi.python.org/pypi/chainercv) and I'm thinking how to integrate student who start with these library, in to robotics research. They loves python development ecosystem, so it seems hard for them to use ROS-development systems, but we as ROS users, would like to use their results on our ROS development systems easily. So for me, if there is an bloom-tools that can package 3rd python package would be great, like we do for cmake package with http://wiki.ros.org/bloom/Tutorials/ReleaseThirdParty method. But I still have concern that their python package has dependency on other pip library, which may differ from Ubuntu-default python library and mixing environment with pip and ubuntu has a lot of troubles, so another idea for me is to extend roslaunch/rosrun to run package level pyenv , before they run python node which depend on other pip library. |
@k-okada My usecase for catkin_pip seems similar to yours :
I'm currently working like that, so think it will do the job fine for you. Please do leave issues if it doesn't. Example : https://github.com/asmodehn/pyros-config released in ros with https://github.com/asmodehn/pyros-config-rosrelease But there are many incompatibilities between ROS and python workflows, and catkin_pip only takes care of building and packaging. I have other packages in development (https://github.com/asmodehn/pyros and other related repos) to help during development and follow the "pythonic way". Just let me know if you want to discuss these. |
@asmodehn Thanks for comment, and as I already send some PRs, this is (mostly) exactly what I need.
I'm personally not familiar with "pythonic way" development, but does
seems ok to them? and if we can write something like
Then, it's perfect for me. I have sent very primitive PR to pyros-dev/catkin_pip#98 |
@k-okada It seems you are following the same process than what I did at first : trying to encapsulate what you don't know (python package) inside what you know (ros package).
Putting all python packages inside one ros package just adds another layer of complexity on top of workspaces stack (which is already more complex than usual python workflow). Until ROS has a better integration with python (integrated interpreter + custom ROS PyPI for example), the simplest way is to match ROS way of doing things :
I wrote a small doc about that in the catkin_pip documentation This way :
To be able to develop using the "pythonic way" more things are needed (pyros-*). But that is independent of bloom and the problem for dependencies, so we can talk about it in another place. |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
…-generate rosdebian
…, this will solve the problem such as: The following packages have unmet dependencies: ros-one-jsk-data : Depends: gdown but it is not installable where gdown is defined as pip package. See https://github.com/ros/rosdistro/blob/0adeee9a8a0fd2825b47aeeb3efeffba3465774c/rosdep/python.yaml#L1667-L1670
…, this will solve the problem such as: The following packages have unmet dependencies: ros-one-jsk-data : Depends: gdown but it is not installable where gdown is defined as pip package. See https://github.com/ros/rosdistro/blob/0adeee9a8a0fd2825b47aeeb3efeffba3465774c/rosdep/python.yaml#L1667-L1670
I now that having pip rosdep entory in the package to be released is not a good manner, but just in case that we have such package and still would like to release them, I think this option reduces maintenance costs very much.