search

ros2 / launch

Tools for launching multiple processes and for writing tests involving multiple processes.
Apache License 2.0
126 stars 144 forks source link

Add consider_namespace_packages=False #766

Closed tonynajjar closed 8 months ago

tonynajjar commented 8 months ago

Fixes https://github.com/ros2/launch/issues/765. That's a quick patch to avoid an error but I don't know if it's the "cleanest" solution, please advise.

Needs backporting humble and iron

clalancette commented 8 months ago

CI:

tonynajjar commented 8 months ago

@clalancette success!

tonynajjar commented 8 months ago

@clalancette this should be backported right? I faced the issue on humble

clalancette commented 8 months ago

@clalancette this should be backported right? I faced the issue on humble

Do we really need it there? We know for sure Humble and Iron won't run into this. They are pinned to older versions of pytest.

tonynajjar commented 8 months ago

Do we really need it there? We know for sure Humble and Iron won't run into this.

In the original issue @jtbandes mentioned A test failure was encountered here (for Humble, Iron, and Rolling):

They are pinned to older versions of pytest.

Can you point to where that pin is configured?

xydesa commented 8 months ago

My team is mostly running on humble and running into this issue constantly. We do pip install pytest.

tonynajjar commented 8 months ago

@clalancette I think I have a hint of what's happening, at least for my setup, but I'm assuming it's a similar issue for the others.

In my dockerfile I have pip install -U git+https://github.com/colcon/colcon-core.git@master, and for some reason this upgrades pytest from 6.2.5 to 8.1.1 which makes this bug appear on humble. I guess others also have some pip installations that upgrades their pytest. Please note that pytest 6.2.5 is already installed via pip in the official ros image osrf/ros:humble-desktop-full

At first I assumed that this was expected because the master branch of colcon-core might be tracking the latest versions of pytest, however it reproduces even with pip install -U git+https://github.com/colcon/colcon-core.git@0.15.2 and that is the version of colcon-core that's installed on humble with apt.

I always hear that mixing pip and apt installations is not a good idea...

@cottsay maybe you have an idea?

cottsay commented 8 months ago

In my dockerfile I have pip install -U git+https://github.com/colcon/colcon-core.git@master, and for some reason this upgrades pytest from 6.2.5 to 8.1.1 which makes this bug appear on humble.

The -U flag to pip install makes it upgrade to the latest version of all packages in the dependency chain. Since colcon-core depends on pytest, it gets included. It has nothing to do with targeting master of colcon-core.

I always hear that mixing pip and apt installations is not a good idea...

The python team is doing quite a lot to try and drive this point home.

tonynajjar commented 8 months ago

:facepalm: I should have noticed that capital U when copying the command from here, I assumed that was the argument needed to pip install directly from a repo. Is it really necessary then or better to remove it from the documentation?

However, even without the -U, pytest still gets upgraded. Am I doing something else wrong?

tony@tony-xmg-22:~$ docker run -it --rm osrf/ros:humble-desktop-full
root@8fb95fdd108c:/# sudo apt update
Get:1 http://security.ubuntu.com/ubuntu jammy-security InRelease [110 kB]
Get:2 http://archive.ubuntu.com/ubuntu jammy InRelease [270 kB]                                                                     
Get:3 http://packages.ros.org/ros2/ubuntu jammy InRelease [4682 B]
Get:4 http://security.ubuntu.com/ubuntu jammy-security/restricted amd64 Packages [1960 kB]
Get:5 http://packages.ros.org/ros2/ubuntu jammy/main amd64 Packages [1459 kB]
Get:6 http://security.ubuntu.com/ubuntu jammy-security/main amd64 Packages [1569 kB]               
Get:7 http://archive.ubuntu.com/ubuntu jammy-updates InRelease [119 kB]                                                 
Get:8 http://security.ubuntu.com/ubuntu jammy-security/multiverse amd64 Packages [44.6 kB]        
Get:9 http://security.ubuntu.com/ubuntu jammy-security/universe amd64 Packages [1079 kB]                                         
Get:10 http://archive.ubuntu.com/ubuntu jammy-backports InRelease [109 kB]                                                                 
Get:11 http://archive.ubuntu.com/ubuntu jammy/universe amd64 Packages [17.5 MB]     
Get:12 http://archive.ubuntu.com/ubuntu jammy/main amd64 Packages [1792 kB]      
Get:13 http://archive.ubuntu.com/ubuntu jammy/restricted amd64 Packages [164 kB]           
Get:14 http://archive.ubuntu.com/ubuntu jammy/multiverse amd64 Packages [266 kB]            
Get:15 http://archive.ubuntu.com/ubuntu jammy-updates/main amd64 Packages [1848 kB]
Get:16 http://archive.ubuntu.com/ubuntu jammy-updates/universe amd64 Packages [1352 kB]
Get:17 http://archive.ubuntu.com/ubuntu jammy-updates/restricted amd64 Packages [1997 kB]
Get:18 http://archive.ubuntu.com/ubuntu jammy-updates/multiverse amd64 Packages [50.4 kB]
Get:19 http://archive.ubuntu.com/ubuntu jammy-backports/main amd64 Packages [80.9 kB]
Get:20 http://archive.ubuntu.com/ubuntu jammy-backports/universe amd64 Packages [33.3 kB]
Fetched 31.8 MB in 7s (4703 kB/s)                                                                                                                            
Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
16 packages can be upgraded. Run 'apt list --upgradable' to see them.
root@8fb95fdd108c:/# ^C
root@8fb95fdd108c:/# ^C
root@8fb95fdd108c:/# ^C
root@8fb95fdd108c:/# ^C
root@8fb95fdd108c:/# ^C
root@8fb95fdd108c:/# sudo apt install -y --no-install-recommends python3-pip 
Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
The following additional packages will be installed:
  python3-wheel
The following NEW packages will be installed:
  python3-pip python3-wheel
0 upgraded, 2 newly installed, 0 to remove and 16 not upgraded.
Need to get 1337 kB of archives.
After this operation, 7178 kB of additional disk space will be used.
Get:1 http://archive.ubuntu.com/ubuntu jammy-updates/universe amd64 python3-wheel all 0.37.1-2ubuntu0.22.04.1 [32.0 kB]
Get:2 http://archive.ubuntu.com/ubuntu jammy-updates/universe amd64 python3-pip all 22.0.2+dfsg-1ubuntu0.4 [1305 kB]
Fetched 1337 kB in 1s (1341 kB/s)     
debconf: delaying package configuration, since apt-utils is not installed
Selecting previously unselected package python3-wheel.
(Reading database ... 130976 files and directories currently installed.)
Preparing to unpack .../python3-wheel_0.37.1-2ubuntu0.22.04.1_all.deb ...
Unpacking python3-wheel (0.37.1-2ubuntu0.22.04.1) ...
Selecting previously unselected package python3-pip.
Preparing to unpack .../python3-pip_22.0.2+dfsg-1ubuntu0.4_all.deb ...
Unpacking python3-pip (22.0.2+dfsg-1ubuntu0.4) ...
Setting up python3-wheel (0.37.1-2ubuntu0.22.04.1) ...
Setting up python3-pip (22.0.2+dfsg-1ubuntu0.4) ...
root@8fb95fdd108c:/# 
root@8fb95fdd108c:/# 
root@8fb95fdd108c:/# pip show pytest
Name: pytest
Version: 6.2.5
Summary: pytest: simple powerful testing with Python
Home-page: https://docs.pytest.org/en/latest/
Author: Holger Krekel, Bruno Oliveira, Ronny Pfannschmidt, Floris Bruynooghe, Brianna Laugher, Florian Bruhin and others
Author-email: 
License: MIT
Location: /usr/lib/python3/dist-packages
Requires: 
Required-by: colcon-core
root@8fb95fdd108c:/# 
root@8fb95fdd108c:/# 
root@8fb95fdd108c:/# 
root@8fb95fdd108c:/# pip install --no-cache-dir git+https://github.com/colcon/colcon-core.git@0.15.2
Collecting git+https://github.com/colcon/colcon-core.git@0.15.2
  Cloning https://github.com/colcon/colcon-core.git (to revision 0.15.2) to /tmp/pip-req-build-vi7huhuc
  Running command git clone --filter=blob:none --quiet https://github.com/colcon/colcon-core.git /tmp/pip-req-build-vi7huhuc
  Running command git checkout -q 67de0a35b9214bd69983bd62fb51de6a83c092e0
  Resolved https://github.com/colcon/colcon-core.git to commit 67de0a35b9214bd69983bd62fb51de6a83c092e0
  Preparing metadata (setup.py) ... done
Requirement already satisfied: EmPy<4 in /usr/lib/python3/dist-packages (from colcon-core==0.15.2) (3.3.4)
Requirement already satisfied: distlib in /usr/lib/python3/dist-packages (from colcon-core==0.15.2) (0.3.4)
Requirement already satisfied: packaging in /usr/lib/python3/dist-packages (from colcon-core==0.15.2) (21.3)
Requirement already satisfied: pytest in /usr/lib/python3/dist-packages (from colcon-core==0.15.2) (6.2.5)
Collecting pytest-cov
  Downloading pytest_cov-4.1.0-py3-none-any.whl (21 kB)
Collecting pytest-repeat
  Downloading pytest_repeat-0.9.3-py3-none-any.whl (4.2 kB)
Collecting pytest-rerunfailures
  Downloading pytest_rerunfailures-14.0-py3-none-any.whl (12 kB)
Requirement already satisfied: setuptools>=30.3.0 in /usr/lib/python3/dist-packages (from colcon-core==0.15.2) (59.6.0)
Collecting coverage[toml]>=5.2.1
  Downloading coverage-7.4.3-cp310-cp310-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_17_x86_64.manylinux2014_x86_64.whl (234 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 234.1/234.1 KB 2.9 MB/s eta 0:00:00
Collecting pytest
  Downloading pytest-8.1.1-py3-none-any.whl (337 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 337.4/337.4 KB 9.5 MB/s eta 0:00:00
Requirement already satisfied: iniconfig in /usr/lib/python3/dist-packages (from pytest->colcon-core==0.15.2) (1.1.1)
Collecting pluggy<2.0,>=1.4
  Downloading pluggy-1.4.0-py3-none-any.whl (20 kB)
Collecting exceptiongroup>=1.0.0rc8
  Downloading exceptiongroup-1.2.0-py3-none-any.whl (16 kB)
Collecting tomli>=1
  Downloading tomli-2.0.1-py3-none-any.whl (12 kB)
Installing collected packages: tomli, pluggy, exceptiongroup, coverage, pytest, pytest-rerunfailures, pytest-repeat, pytest-cov
  Attempting uninstall: pluggy
    Found existing installation: pluggy 0.13.0
    Not uninstalling pluggy at /usr/lib/python3/dist-packages, outside environment /usr
    Can't uninstall 'pluggy'. No files were found to uninstall.
  Attempting uninstall: pytest
    Found existing installation: pytest 6.2.5
    Not uninstalling pytest at /usr/lib/python3/dist-packages, outside environment /usr
    Can't uninstall 'pytest'. No files were found to uninstall.
Successfully installed coverage-7.4.3 exceptiongroup-1.2.0 pluggy-1.4.0 pytest-8.1.1 pytest-cov-4.1.0 pytest-repeat-0.9.3 pytest-rerunfailures-14.0 tomli-2.0.1
WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv
root@8fb95fdd108c:/# pip show pytest
Name: pytest
Version: 8.1.1
Summary: pytest: simple powerful testing with Python
Home-page: 
Author: Holger Krekel, Bruno Oliveira, Ronny Pfannschmidt, Floris Bruynooghe, Brianna Laugher, Florian Bruhin, Others (See AUTHORS)
Author-email: 
License: MIT
Location: /usr/local/lib/python3.10/dist-packages
Requires: exceptiongroup, iniconfig, packaging, pluggy, tomli
Required-by: colcon-core, pytest-cov, pytest-repeat, pytest-rerunfailures
root@8fb95fdd108c:/#
jtbandes commented 8 months ago

In my case, the workflow is "just" running ros-tooling/action-ros-ci: https://github.com/foxglove/schemas/blob/2417be2ba8fdb8174bc69cc7da450f302ac46396/.github/workflows/ci.yml#L183-L189

cottsay commented 8 months ago

I should have noticed that capital U when copying the command from here, I assumed that was the argument needed to pip install directly from a repo. Is it really necessary then or better to remove it from the documentation?

It isn't typically feasible to test all combinations of all recursive dependencies for a project. For colcon's CI, we test using the latest releases of all dependencies, so it makes sense to recommend the same practice for those installing using pip. Clearly there was a bug here that made it incompatible with newer pytest independent of colcon or its direct requirements. If you want a blessed set of known working version combinations, well that's what the system package manager is for and is one of many arguments to use that instead of pip.

However, even without the -U, pytest still gets upgraded. Am I doing something else wrong?

I would guess that one of the pytest plugins that are getting installed requires a version newer than 6.2.5. While omitting -U will not upgrade satisfied dependencies unprovoked, pip won't install an older version of an unsatisfied requirement just to avoid upgrading another dependency. You can confirm this by installing the pytest plugins individually to see which one brings the upgrade. Installing the deb for that plugin, if available, would avoid the need to install it and subsequently upgrade pytest.

If you don't have a really good reason to install colcon from pip, I recommend using your system's package manager to avoid these problems.

tonynajjar commented 8 months ago

Thanks a lot for the detailed explanation.

If you don't have a really good reason to install colcon from pip

My "really good reason" was to use features on master not yet released but now 0.16.0 is released so I'll wait for the apt version.

Anyway I don't want to hijack the discussion with my "personal setup problems", for others having this issue maybe try to find out at what point your pytest is getting upgraded involuntarily

sea-bass commented 8 months ago

@clalancette this should be backported right? I faced the issue on humble

Do we really need it there? We know for sure Humble and Iron won't run into this. They are pinned to older versions of pytest.

FWIW I just ran into this on Humble, but because I have a "hybrid" setup that has both a pure Python package and a ROS wrapper around it. I can live without it, but would appreciate a backport.

tonynajjar commented 7 months ago

@clalancette seems like multiple people would benefit from that backport even if it's not strictly required, can we have it if it has no downsides?

MichaelOrlov commented 6 months ago

@clalancette I am getting the same error

2024-05-06T06:15:52.1234322Z ../../../../build/launch_testing/launch_testing/pytest/hooks.py:188: in pytest_pycollect_makemodule
2024-05-06T06:15:52.1234947Z     entrypoint = find_launch_test_entrypoint(path)
2024-05-06T06:15:52.1235552Z ../../../../build/launch_testing/launch_testing/pytest/hooks.py:178: in find_launch_test_entrypoint
2024-05-06T06:15:52.1236127Z     module = import_path(path, root=None)
2024-05-06T06:15:52.1236788Z E   TypeError: import_path() missing 1 required keyword-only argument: 'consider_namespace_packages'
2024-05-06T06:15:52.1237861Z --- generated xml file: /__w/rosbag2/rosbag2/ros_ws/build/ros2bag/pytest.xml ---

in build_and_test CI job on Iron. It seems pytest is not pinned.

tblom commented 6 months ago

We also run into this with Humble. Will this be backported?

MichaelOrlov commented 6 months ago
clalancette commented 6 months ago

@Mergifyio backport humble iron

mergify[bot] commented 6 months ago

backport humble iron

✅ Backports have been created

* [#777 Add consider_namespace_packages=False (backport #766)](https://github.com/ros2/launch/pull/777) has been created for branch `humble` * [#778 Add consider_namespace_packages=False (backport #766)](https://github.com/ros2/launch/pull/778) has been created for branch `iron`