support version specific install for pip (and others) by k-okada · Pull Request #694 · ros-infrastructure/rosdep

k-okada · 2019-07-18T10:03:27Z

We often have trouble on version problem of pip installed modules, and to avoid this it is very impotant to not to upgrade the python module version automatically (ofcourse do not use pip, is the best solution....)

This PR respect version_eq or other attribute in depend tag when running pip install

  <exec_depend version_lte="5.7">ipython-pip</exec_depend>
  <exec_depend version_eq="4.10">ipykernel-pip</exec_depend>

NikolausDemmel · 2019-07-18T10:50:45Z

I'm not maintaining rosdep, nor using it much these days, but just some thoughts from when we had discussed this in the past:

What happens if an incompatible version is already installed?
What happens if you have different version statements (conflicting or compatible) for the same pip package in multiple ROS packages?

You might argue to just leave all those decisions to pip, and that is probably a good idea if possible, but consider the following: I'm not 100% sure here, I believe rosdep tries to combine multiple packages to a single pip install command, if no other package managers appear in between in the list of resolutions. But what if they get split into separate invocations? So one case would be two different version requirements (compatible or not) for a pip package in the same call to pip install, and another case might be two different version requirements (compatible or not) for a pip package in two subsequent calls pip install. In the latter, might pip uninstall the package from the first invocation and make it silently incompatible?

Ideally, the intended behaviour in such cases would be documented and tested. But maybe it's also ok to keep it simple if it covers >95% of use cases. You should probably wait for some feedback on willingness/bandwidth to include this change from a maintainer.

codecov-io · 2019-07-20T03:20:14Z

Codecov Report

Merging #694 into master will increase coverage by 0.30%.
The diff coverage is 90.32%.

@@            Coverage Diff             @@
##           master     #694      +/-   ##
==========================================
+ Coverage   74.85%   75.16%   +0.30%     
==========================================
  Files          41       41              
  Lines        3214     3277      +63     
==========================================
+ Hits         2406     2463      +57     
- Misses        808      814       +6

Impacted Files	Coverage Δ
src/rosdep2/catkin_support.py	`46.55% <0.00%> (ø)`
src/rosdep2/lookup.py	`87.68% <85.71%> (-0.15%)`	⬇️
src/rosdep2/platforms/pip.py	`73.72% <91.48%> (+10.21%)`	⬆️
src/rosdep2/rospkg_loader.py	`93.97% <94.11%> (-0.23%)`	⬇️
src/rosdep2/installers.py	`78.27% <100.00%> (ø)`
src/rosdep2/main.py	`48.83% <100.00%> (ø)`
src/rosdep2/platforms/osx.py	`51.93% <100.00%> (ø)`
src/rosdep2/platforms/source.py	`90.55% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1784da...6d09a34. Read the comment docs.

130s · 2019-07-20T15:32:26Z

Just raising +1 flag for adding this capability to utilize package.xml being able to define version limitations. We've ended up runningpip manually for pkgs that need to be specific versions, which is obviously tedious and turned out to be prone to human errors. Concerns raised by @NikolausDemmel makes sense though.

knorth55 · 2019-12-16T15:53:54Z

+1 for this PR, but I can understand @NikolausDemmel 's concern.
But there will be the EOL of python2 soon, so we should move forward for this discussion because some python packages are modified to run only on python3 but carelessly released on python2.
In that case, I want to use version information for rosdep, too.

…p, some package requries python3

Timple · 2021-02-16T09:18:07Z

some python packages are modified to run only on python3 but carelessly released on python2.

In that case I think it would be okay to just pin the version in the rosdistro file because everybody on python2 will suffer from this.

russkel · 2022-06-15T02:14:54Z

src/rosdep2/installers.py

        return []  # Default return empty list

-    def resolve(self, rosdep_args_dict):
+    def resolve(self, rosdep, rosdep_args_dict):


Can you please move this argument to after the rosdep_args_dict and add a default value for it so it doesn't unnecessarily break other projects that rely on resolve?

@russkel thanks for comment, this makes much simpler, fixed the code

src/rosdep2/installers.py

russkel · 2022-06-15T02:16:56Z

src/rosdep2/lookup.py

        return resolutions_flat, errors

-    def resolve(self, rosdep_key, resource_name, installer_context):
+    def resolve(self, rosdep, resource_name, installer_context):


this is not new argument, just to rename variable name, since we have changed from list of keys to catkin_pkg.package.Dependency object
:param rosdep_key: rosdep key to resolve :param rosdeps: rosdep key (catkin_pkg.package.Dependency object) to resolve

Yep understood.

russkel · 2022-06-15T02:34:03Z

Alternatively, could you store the Dependency object in the rosdep_args_dict instead of changing the arguments of the resolve method?

Edit: disregard above comment. I investigated it and it wasn't that simple.

codecov-commenter · 2022-06-16T01:33:49Z

Codecov Report

Merging #694 (6d09a34) into master (b2ee64f) will increase coverage by 0.33%.
The diff coverage is 90.32%.

❗ Current head 6d09a34 differs from pull request most recent head 4749394. Consider uploading reports for the commit 4749394 to get more accurate results

@@            Coverage Diff             @@
##           master     #694      +/-   ##
==========================================
+ Coverage   74.82%   75.16%   +0.33%     
==========================================
  Files          44       41       -3     
  Lines        3360     3277      -83     
==========================================
- Hits         2514     2463      -51     
+ Misses        846      814      -32

Impacted Files	Coverage Δ
src/rosdep2/catkin_support.py	`46.55% <0.00%> (ø)`
src/rosdep2/lookup.py	`87.68% <85.71%> (-0.50%)`	⬇️
src/rosdep2/platforms/pip.py	`73.72% <91.48%> (+9.72%)`	⬆️
src/rosdep2/rospkg_loader.py	`93.97% <94.11%> (-0.47%)`	⬇️
src/rosdep2/installers.py	`78.27% <100.00%> (ø)`
src/rosdep2/main.py	`48.83% <100.00%> (+0.22%)`	⬆️
src/rosdep2/platforms/osx.py	`51.93% <100.00%> (ø)`
src/rosdep2/platforms/source.py	`90.55% <100.00%> (+0.10%)`	⬆️
src/rosdep2/platforms/gem.py	`78.37% <0.00%> (-6.24%)`	⬇️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2ee64f...4749394. Read the comment docs.

…ep_args_dict adn add default value for backward compatibility

russkel · 2022-06-16T02:44:54Z

test/test_rosdep_lookup.py

    print(lookup.get_rosdeps('stack1_p1'))
-    assert set([d.name for d in lookup.get_rosdeps('stack1_p1')]) == set(['stack1_dep1', 'stack1_p1_dep1', 'stack1_p1_dep2'])
-    assert set([d.name for d in lookup.get_rosdeps('stack1_p1', implicit=False)]) == set(['stack1_dep1', 'stack1_p1_dep1', 'stack1_p1_dep2'])
+    assert set({d.name for d in lookup.get_rosdeps('stack1_p1')}) == set(['stack1_dep1', 'stack1_p1_dep1', 'stack1_p1_dep2'])


Set comprehension doesn't need the outer set()

…ion errors

JWhitleyWork · 2022-10-06T21:49:19Z

@k-okada Would you mind taking a look at my new draft #901 which covers a small portion of what you've done here but on the install-verification side? This at least allows you to specify pip entries in YAML files which have a version requirement and verify that they were installed with that specific version.

k-okada force-pushed the pip_version branch from 1529999 to d6a6cd2 Compare July 18, 2019 10:55

k-okada mentioned this pull request Jul 18, 2019

環境構築方法について YOODS/rovi#51

Merged

k-okada force-pushed the pip_version branch 3 times, most recently from 9415aee to 604affe Compare July 19, 2019 08:20

k-okada force-pushed the pip_version branch from 64d1f9c to 0a74a59 Compare July 20, 2019 04:50

k-okada mentioned this pull request Nov 5, 2019

[jsk_perception] Support chainercv mask rcnn jsk-ros-pkg/jsk_recognition#2435

Merged

knorth55 mentioned this pull request Dec 16, 2019

set chainer version less than 7.0.0 jsk-ros-pkg/jsk_recognition#2480

Closed

k-okada added a commit to k-okada/jsk_recognition that referenced this pull request Dec 21, 2019

use ros-infrastructure/rosdep#694 to respect version_lt for python pi…

2862ee9

…p, some package requries python3

k-okada mentioned this pull request Dec 21, 2019

set chainer version less than 7.0.0 jsk-ros-pkg/jsk_recognition#2485

Merged

k-okada added a commit to k-okada/jsk_recognition that referenced this pull request Dec 22, 2019

use ros-infrastructure/rosdep#694 to respect version_lt for python pi…

de27efd

…p, some package requries python3

pazeshun mentioned this pull request Feb 11, 2020

Incompatible version is released to Python 2 chainer/chainer#8545

Closed

k-okada added a commit to k-okada/jsk_recognition that referenced this pull request May 9, 2020

use ros-infrastructure/rosdep#694 to respect version_lt for python pi…

b136bc8

…p, some package requries python3

k-okada force-pushed the pip_version branch from 618583a to fc816c5 Compare May 10, 2020 04:54

k-okada added a commit to k-okada/jsk_recognition that referenced this pull request May 10, 2020

use ros-infrastructure/rosdep#694 to respect version_lt for python pi…

d20aaae

…p, some package requries python3

k-okada added a commit to k-okada/jsk_recognition that referenced this pull request May 10, 2020

use ros-infrastructure/rosdep#694 to respect version_lt for python pi…

cc2b918

…p, some package requries python3

k-okada added a commit to k-okada/jsk_recognition that referenced this pull request May 10, 2020

use ros-infrastructure/rosdep#694 to respect version_lt for python pi…

3789165

…p, some package requries python3

russkel reviewed Jun 15, 2022

View reviewed changes

src/rosdep2/installers.py Outdated Show resolved Hide resolved

russkel reviewed Jun 15, 2022

View reviewed changes

k-okada added 3 commits June 16, 2022 10:45

support veresion specific install for pip (and other installeres)

e0f3db6

installer.resolve takes two arguments

ffdef47

rosdep2/lookup.py: self.resolve requries Dependency

bfb1d4b

k-okada added 2 commits June 16, 2022 10:45

rosdep resovle retuns pkg<=0.0 not 'pkg<=0.0'

409c435

resolve(self, rosdep,rosdep_arg_dict) move rosdep argument after rosd…

eff0592

…ep_args_dict adn add default value for backward compatibility

k-okada force-pushed the pip_version branch from 8e1e84c to eff0592 Compare June 16, 2022 01:45

russkel reviewed Jun 16, 2022

View reviewed changes

fix C403 Unnecessary list comprehension - rewrite as a set comprehens…

4749394

…ion errors

k-okada force-pushed the pip_version branch from 30edbb6 to 4749394 Compare June 16, 2022 03:09

knorth55 mentioned this pull request Dec 6, 2022

[panda_eus] Skip generating dual_panda.l when franka_description or xacro is old to avoid build failure jsk-ros-pkg/jsk_robot#1751

Merged

Conversation

k-okada commented Jul 18, 2019

Uh oh!

NikolausDemmel commented Jul 18, 2019

Uh oh!

codecov-io commented Jul 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

130s commented Jul 20, 2019

Uh oh!

knorth55 commented Dec 16, 2019

Uh oh!

Timple commented Feb 16, 2021

Uh oh!

russkel Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

k-okada Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

russkel Jun 15, 2022

Choose a reason for hiding this comment

Uh oh!

k-okada Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

russkel Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

russkel commented Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

russkel Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

JWhitleyWork commented Oct 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

codecov-io commented Jul 20, 2019 •

edited

Loading

russkel commented Jun 15, 2022 •

edited

Loading

codecov-commenter commented Jun 16, 2022 •

edited

Loading