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

Tox is unable to discover all the deps across multiple extras #3433

Open
webknjaz opened this issue Oct 31, 2024 · 8 comments
Open

Tox is unable to discover all the deps across multiple extras #3433

webknjaz opened this issue Oct 31, 2024 · 8 comments

Comments

@webknjaz
Copy link
Contributor

webknjaz commented Oct 31, 2024

Imagine the extras declared like this:

# pyproject.toml

[project.optional-dependencies]
extra-1 = ["pkg-a", "pkg-b"]
extra-2 = ["pkg-b", "pkg-c"]
extra-3 = ["pkg-d", "pkg-e"]

And tox is referencing them:

# tox.ini

extras =
  extra-1
  extra-2

I got into a situation where import pkg_d, pkg_e was failing: https://github.com/ansible/awx-plugins/actions/runs/11597267178/job/32290438874?pr=52#step:18:61.

With some debugging and staring at #2603, I discovered that the logic in https://github.com/tox-dev/tox/blob/5d880fc/src/tox/tox_env/python/virtual_env/package/util.py#L28 is flawed: when I put a breakpoint at the end of that function, I saw that the result only contains ['pkg-a', 'pkg-b']. This left me puzzled, but I already had my suspicions regarding only one of the extras contributing to the list of dependencies, having observed that some of the deps appear in the env.
When I looked closer, I noticed that conditional if todo & extra_markers: and it explained what was happening: & operation on set()s returns only items that exist in both sets. So on the first iteration it could be {"pkg-a", "pkg-b"} & {"pkg-b", "pkg-c"} == {"pkg-a", "pkg-b"}, while on the following one it could be {"pkg-b", "pkg-c"} & {"pkg-d", "pkg-e"} == set(). And depending on the order of operations, a subset of extras would skip that entire if-block, not contributing any dependencies to the list.

I experimented with changing if todo & extra_markers: to if todo | extra_markers: and it helped since it's a union, not intersection. But that raised questions regarding the necessity of todo in that check. The thing is that there's while todo: a few lines above, meaning that todo is always non-empty and that check would then always be if True:. So I've gone further and changed it to just if extra_markers:. It works for me and does not seem to have any side effects. But I haven't attempted running tox's test suite against my patch.

I'm not sure if I'll get to looking into it more this week, so I'm doing the next best thing and document this observation on the tracker…

@gaborbernat
Copy link
Member

That todo is there for circular extras I believe.

@webknjaz
Copy link
Contributor Author

@gaborbernat but it looks like todo is still used outside the nested for-loop, within the while-loop. Removing it from this specific condition doesn't prevent handling recursive extras. Instead, it makes more cases unskipped.

@webknjaz
Copy link
Contributor Author

webknjaz commented Nov 1, 2024

UPD: this change does break the recursive extras tests.

@webknjaz
Copy link
Contributor Author

webknjaz commented Nov 1, 2024

UPD: this seems to be related to extra names containing underscores.

@webknjaz
Copy link
Contributor Author

webknjaz commented Nov 2, 2024

Minimal reproducer:

# pyproject.toml

[build-system]
requires = ["setuptools >= 64"]
build-backend = "setuptools.build_meta"

[project]
name = "tox-bug-3433-multiple-extras-merge"
version = "1.0.0"
dependencies = []

[project.optional-dependencies]
kebab-case = [
  "boltons",
  "certifi",
]
snake_case = [
  "boltons",
  "littleutils",
  "typing-extensions",
  "pypi-alias",
]
kebab-case-2 = [
  "boltons",
  "tidi",
]
# tox.ini

[testenv]
deps =
  pytest
extras =
  kebab-case
  snake_case
  kebab-case-2
commands =
  {envpython} \
    -bb \
    -E -s -I \
    -Werror \
    -m pytest \
      -v
# repro_test.py

def test_boltons():
    import boltons

def test_littleutils():
    import littleutils

def test_typing_extensions():
    import typing_extensions

def test_pypi_alias():
    import pypi_alias

def test_certifi():
    import certifi

def test_tidi():
    import tidi
$ tox r -e py -r
py: remove tox env folder ~/src/experiments/tox-bug-3433-multiple-extras-merge/.tox/py
.pkg: remove tox env folder ~/src/experiments/tox-bug-3433-multiple-extras-merge/.tox/.pkg
py: install_deps> python -I -m pip install pytest
.pkg: install_requires> python -I -m pip install 'setuptools>=64'
.pkg: _optional_hooks> python ~/src/experiments/tox-bug-3433-multiple-extras-merge/some-venv/lib/python3.13/site-packages/pyproject_api/_backend.py True setuptools.build_meta
.pkg: get_requires_for_build_sdist> python ~/src/experiments/tox-bug-3433-multiple-extras-merge/some-venv/lib/python3.13/site-packages/pyproject_api/_backend.py True setuptools.build_meta
.pkg: build_sdist> python ~/src/experiments/tox-bug-3433-multiple-extras-merge/some-venv/lib/python3.13/site-packages/pyproject_api/_backend.py True setuptools.build_meta
py: install_package_deps> python -I -m pip install boltons certifi tidi
py: install_package> python -I -m pip install --force-reinstall --no-deps ~/src/experiments/tox-bug-3433-multiple-extras-merge/.tox/.tmp/package/22/tox_bug_3433_multiple_extras_merge-1.0.0.tar.gz
py: commands[0]> .tox/py/bin/python -bb -E -s -I -Werror -m pytest -v
=========================================== test session starts ===========================================
platform linux -- Python 3.13.0rc1, pytest-8.3.3, pluggy-1.5.0 -- ~/src/experiments/tox-bug-3433-multiple-extras-merge/.tox/py/bin/python
cachedir: .tox/py/.pytest_cache
rootdir: ~/src/experiments/tox-bug-3433-multiple-extras-merge
configfile: pyproject.toml
collected 6 items                                                                                         

repro_test.py::test_boltons PASSED                                                                  [ 16%]
repro_test.py::test_littleutils FAILED                                                              [ 33%]
repro_test.py::test_typing_extensions FAILED                                                        [ 50%]
repro_test.py::test_pypi_alias FAILED                                                               [ 66%]
repro_test.py::test_certifi PASSED                                                                  [ 83%]
repro_test.py::test_tidi PASSED                                                                     [100%]

================================================ FAILURES =================================================
____________________________________________ test_littleutils _____________________________________________

    def test_littleutils():
>       import littleutils
E       ModuleNotFoundError: No module named 'littleutils'

repro_test.py:5: ModuleNotFoundError
_________________________________________ test_typing_extensions __________________________________________

    def test_typing_extensions():
>       import typing_extensions
E       ModuleNotFoundError: No module named 'typing_extensions'

repro_test.py:8: ModuleNotFoundError
_____________________________________________ test_pypi_alias _____________________________________________

    def test_pypi_alias():
>       import pypi_alias
E       ModuleNotFoundError: No module named 'pypi_alias'

repro_test.py:11: ModuleNotFoundError
========================================= short test summary info =========================================
FAILED repro_test.py::test_littleutils - ModuleNotFoundError: No module named 'littleutils'
FAILED repro_test.py::test_typing_extensions - ModuleNotFoundError: No module named 'typing_extensions'
FAILED repro_test.py::test_pypi_alias - ModuleNotFoundError: No module named 'pypi_alias'
======================================= 3 failed, 3 passed in 0.02s =======================================
py: exit 1 (0.15 seconds) ~/src/experiments/tox-bug-3433-multiple-extras-merge> .tox/py/bin/python -bb -E -s -I -Werror -m pytest -v pid=2905070
  py: FAIL code 1 (5.87=setup[5.72]+cmd[0.15] seconds)
  evaluation failed :( (5.90 seconds)

@webknjaz
Copy link
Contributor Author

webknjaz commented Nov 2, 2024

So my observation is that extras in the wheel metadata (with package = editable) kept underscores. And extras in the sdist (with not package set) had underscores converted to hyphens. But that's setuptools' doing. tox should be able to match them regardless of whether they are normalized. The metadata version setuptools produces is 2.1 which does not yet impose strict limitations on the allowed chars in core packaging metadata, but 2.3 would require that (https://packaging.python.org/en/latest/specifications/core-metadata/#provides-extra-multiple-use / https://peps.python.org/pep-0685/#specification). This means that at some point, there's going to be more cases where new dists are produced with extras only having dashes in metadata, while old dists will also be available with those non-normalized.

I'm not entirely sure where the mismatch is happening. Changing _ to - in both pyproject.toml and tox.ini makes it work. And alternatively, that hack I mentioned above works:

diff --git a/src/tox/tox_env/python/virtual_env/package/util.py b/src/tox/tox_env/python/virtual_env/package/util.py
index add52e58..fa13cb4a 100644
--- a/src/tox/tox_env/python/virtual_env/package/util.py
+++ b/src/tox/tox_env/python/virtual_env/package/util.py
@@ -25,7 +25,7 @@ def dependencies_with_extras_from_markers(
     while todo:
         new_extras: set[str | None] = set()
         for req, extra_markers in deps_with_markers:
-            if todo & extra_markers:
+            if extra_markers:
                 if req.name == package_name:  # support for recursive extras
                     new_extras.update(req.extras or set())
                 else:

I have no idea why either works and not just one of the changes. There must be insufficient normalization present somewhere.

@gaborbernat any ideas where that might be?

@gaborbernat
Copy link
Member

I am not sure why. However, if you put in a poor request with a reproducible and still pass all the tests, I will review and accept it.

@webknjaz
Copy link
Contributor Author

webknjaz commented Nov 4, 2024

poor request

😆

I was thinking maybe an xfailing test for starters… Still have no idea where to start actually fixing it.

webknjaz pushed a commit to jessicamack/awx-plugins that referenced this issue Nov 4, 2024
The extra names in the core packaging metadata are allowed to contain
dashes but not underscores [[1]]. Packaging tooling is supposed to
normalize them if met in the non-metadata configuration but apparently
tox has a bug connected to this, which prevents is from discovering
dependencies provided by extras that contain underscores [[2]].
So as a workaround, this patch uses hyphens in the names of extras,
avoiding any underscores.

Making the direct dependencies optional, means that they are not going
to be picked up by default. To enable pip-tools to include them into
the lock files, this change also updates its configuration file to
instruct it to include dependencies from all the extras present by
always using `--all-extras`. Likewise, to enable tox to install those
dependencies, they are all listed in its configuration file too.

Co-Authored-By: Sviatoslav Sydorenko <[email protected]>

[1]: https://packaging.python.org/en/latest/specifications/core-metadata/#provides-extra-multiple-use
[2]: tox-dev/tox#3433 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants