-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Entries for installing PyTorch by pip #14398
Conversation
python-torch-cuda75-pip: | ||
debian: | ||
pip: | ||
packages: ['http://download.pytorch.org/whl/cu75/torch-0.1.11.post4-cp27-none-linux_x86_64.whl'] |
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 am not sure if rosdep
supports this and if we want to allow this? @ros/ros_team What do you think?
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 think it works at least:
% cat /etc/ros/rosdep/sources.list.d/20-default.list
# os-specific listings first
yaml https://raw.githubusercontent.com/ros/rosdistro/master/rosdep/osx-homebrew.yaml osx
# generic
yaml https://raw.githubusercontent.com/ros/rosdistro/master/rosdep/base.yaml
yaml https://raw.githubusercontent.com/wkentaro/rosdistro/pytorch-entries/rosdep/python.yaml # <- !!!
yaml https://raw.githubusercontent.com/ros/rosdistro/master/rosdep/ruby.yaml
gbpdistro https://raw.githubusercontent.com/ros/rosdistro/master/releases/fuerte.yaml fuerte
# newer distributions (Groovy, Hydro, ...) must not be listed anymore, they are being fetched from the rosdistro index.yaml instead
% cat package.xml
<package>
<name>foo</name>
<version>1.1.1</version>
<description>
foo
</description>
<author email="wada@jsk.imi.i.u-tokyo.ac.jp">Kentaro Wada</author>
<maintainer email="wada@jsk.imi.i.u-tokyo.ac.jp">Kentaro Wada</maintainer>
<license>BSD</license>
<url>http://ros.org/wiki/foo</url>
<buildtool_depend>catkin</buildtool_depend>
<run_depend>python-torch-cuda80-pip</run_depend>
</package>
% rosdep install --from-path . --simulate
#[pip] Installation commands:
sudo -H pip install -U http://download.pytorch.org/whl/cu80/torch-0.1.11.post4-cp27-none-linux_x86_64.whl # <- !!!
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.
@wkentaro Does rosdep
detect that the package is already installed when being invoked again?
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.
No. Because pip
always download package if -U
is specified for installing from url.
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.
Well, I'm not sure why -U
is specified in rosdep
.
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.
-U
means update if there is an update available. It shouldn't reinstall if it's already at the latest version. From the help.
-U, --upgrade Upgrade all specified packages to the newest available version. This process is recursive regardless of whether a dependency
is already satisfied.
If it's reinstalling every time that suggests that it's not detecting it correctly. I think the problem is that using the URL it doesn't know the version requested until after it's already downloaded it.
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.
Yeah, but apt won't upgrade package by default, right?
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.
If you ask apt to install a package it will install the latest version available or upgrade that package if it's already installed. This is slightly different in that it will recursively update packages. But that's because you can't simply do a pip upgrade
like you can do an apt-get upgrade
. The problem is that rosdep should not be invoking the install rule if the package is already installed, because it should detect that it's installed an doesn't need to do so.
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.
If you ask apt to install a package it will install the latest version available or upgrade that package if it's already installed.
Oh, I didn't know that. Thanks.
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.
There may be some technical challenges (see other comment thread) but at the higher level, I don't think that we want to start accepting rules pulling from random urls. Packages hosted in repositories have a review and feedback mechanism. Whereas if we embed a random url we need to provide that review and feedback mechanism. However once the rule is embedded we do not have an effective feedback mechanism. And if any remote server that we reference is compromised rosdep would trust it naively.
I would suggest that if these are packages that are important to have rosdep rules that they be submitted upstream to pypi (which is pretty easy) and then we can add a rule for it pulling from there.
Custom rosdep rules are fully supported. You just add them to your rosdep sources lists in The deprecation that you are referring to is for rosdep rules being hosted inside the package and using the It might make sense for you to create your own rosdep rules file that then you can do things that work for your use cases but may not be completely generic like we would want for public release instead of pushing for these things to be in the public database. |
Yeah, it works on my use case, and I don't need rosdep rule in actual because I can run the pip command by myself. git clone <ROS_PACKAGE>
cd <ROS_PACKAGE>
rosdep install --from-path . -r -y
catkin bt -iv Of course, I can invoke
|
There's a level that convenience is great. But there's also a level of expectations and security. General practice is that you should not install anything during the build. Especially not into the system directories that would require sudo. In your flow for installing you're leaving out a few steps. It's more like this.
The critical steps where you're delegating trust are in the adding the ROS sources. And setting up your rosdep sources, (in that it can pull from non-apt) If you start pulling 3rdparty content during the build. (Especially installing it to the system) You are adding another place where your system security might be compromised. This is why there's a clear separation of adding sources, installation from those sources, and building. To make your use case work it simply requires adding one step to authorize one extra rosdep source. I'd strongly recommend that you stick with the separation and understand that being explicit about these things is although slightly more complicated quite valuable for users. Personally if I was checking out a piece of software and found an installation command in the build steps I'd likely not run the build as I wouldn't necessarily trust that it's not doing other unexpected things to my computer without exhaustively reading the rest of the build process. Which is often prohibitively expensive for checking out new software, thus I'd simply move on. |
I see. I think you're right. |
for jsk-ros-pkg/jsk_recognition#2041
cc @k-okada