Skip to content
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 consider_namespace_packages=False #766

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

tonynajjar
Copy link
Contributor

@tonynajjar tonynajjar commented Mar 12, 2024

Fixes #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

Signed-off-by: Tony Najjar <tony.najjar.1997@gmail.com>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! I left some things that should be fixed to get this in.

launch_testing/launch_testing/pytest/hooks.py Show resolved Hide resolved
Signed-off-by: Tony Najjar <tony.najjar.1997@gmail.com>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, I'll run CI on it next.

@clalancette
Copy link
Contributor

clalancette commented Mar 12, 2024

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@tonynajjar
Copy link
Contributor Author

@clalancette success!

@clalancette clalancette merged commit 07f4332 into ros2:rolling Mar 13, 2024
2 of 3 checks passed
@tonynajjar
Copy link
Contributor Author

tonynajjar commented Mar 13, 2024

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

@clalancette
Copy link
Contributor

@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
Copy link
Contributor Author

tonynajjar commented Mar 13, 2024

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
Copy link

xydesa commented Mar 13, 2024

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

@tonynajjar
Copy link
Contributor Author

tonynajjar commented Mar 13, 2024

@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
Copy link
Member

cottsay commented Mar 13, 2024

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
Copy link
Contributor Author

tonynajjar commented Mar 13, 2024

🤦 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
Copy link
Member

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
Copy link
Member

cottsay commented Mar 13, 2024

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
Copy link
Contributor Author

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
Copy link

sea-bass commented Mar 16, 2024

@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
Copy link
Contributor Author

@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?

tonynajjar added a commit to angsa-robotics/launch that referenced this pull request Apr 5, 2024
* Add consider_namespace_packages=False

Signed-off-by: Tony Najjar <tony.najjar.1997@gmail.com>
@MichaelOrlov
Copy link

@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.

jplapp pushed a commit to pixel-robotics/launch that referenced this pull request May 11, 2024
* Add consider_namespace_packages=False

Signed-off-by: Tony Najjar <tony.najjar.1997@gmail.com>
(cherry picked from commit 07f4332)
@tblom
Copy link

tblom commented May 14, 2024

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

@clalancette
Copy link
Contributor

@Mergifyio backport humble iron

Copy link

mergify bot commented May 14, 2024

backport humble iron

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request May 14, 2024
* Add consider_namespace_packages=False

Signed-off-by: Tony Najjar <tony.najjar.1997@gmail.com>
(cherry picked from commit 07f4332)
mergify bot pushed a commit that referenced this pull request May 14, 2024
* Add consider_namespace_packages=False

Signed-off-by: Tony Najjar <tony.najjar.1997@gmail.com>
(cherry picked from commit 07f4332)
clalancette pushed a commit that referenced this pull request May 15, 2024
* Add consider_namespace_packages=False

Signed-off-by: Tony Najjar <tony.najjar.1997@gmail.com>
(cherry picked from commit 07f4332)

Co-authored-by: Tony Najjar <tony.najjar.1997@gmail.com>
audrow pushed a commit that referenced this pull request May 16, 2024
* Add consider_namespace_packages=False

Signed-off-by: Tony Najjar <tony.najjar.1997@gmail.com>
(cherry picked from commit 07f4332)

Co-authored-by: Tony Najjar <tony.najjar.1997@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants