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

[Bug]: poetry incorrectly tries to resolve an unnecessary importlib-metadata dependency #1311

Closed
cwegener opened this issue Jan 6, 2023 · 6 comments · Fixed by #1345
Closed
Assignees
Labels
kind/Bug Something isn't working valuestream/SDK

Comments

@cwegener
Copy link

cwegener commented Jan 6, 2023

Singer SDK Version

0.16.0

Python Version

3.10

Bug scope

Other

Operating System

Linux

Description

After the bump of tox from version 3 to version 4 in the cookiecutter template, poetry is now getting its knickers in a twist about being unable to resolve a suitable version of importlib-metadata even though importlib-metadata has a marker on it for Python 3.7 and older only.

This seems to be as a result of #1305

Steps to reproduce:

  1. Create new Singer Tap project with the new tox version 4 dev dependency using poetry:
poetry init -n --name=test --description=Test --license=MIT --python='<3.12,>=3.7.1'  --dependency='singer-sdk:^0.16.0' --dev-dependency='tox:^4.1.3'
  1. Run poetry install

Expected Result

poetry install completest successfully

Actual Result

Resolving dependencies... (1.3s)

Because no versions of tox match >4.1.3,<4.2.0 || >4.2.0,<4.2.1 || >4.2.1,<4.2.2 || >4.2.2,<4.2.3 || >4.2.3,<4.2.4 || >4.2.4,<5.0.0
 and tox (4.1.3) depends on importlib-metadata (>=5.2), tox (>=4.1.3,<4.2.0 || >4.2.0,<4.2.1 || >4.2.1,<4.2.2 || >4.2.2,<4.2.3 || >4.2.3,<4.2.4 || >4.2.4,<5.0.0) requires importlib-metadata (>=5.2).
And because tox (4.2.0) depends on importlib-metadata (>=5.2), tox (>=4.1.3,<4.2.1 || >4.2.1,<4.2.2 || >4.2.2,<4.2.3 || >4.2.3,<4.2.4 || >4.2.4,<5.0.0) requires importlib-metadata (>=5.2).
And because tox (4.2.1) depends on importlib-metadata (>=5.2)
 and tox (4.2.2) depends on importlib-metadata (>=5.2), tox (>=4.1.3,<4.2.3 || >4.2.3,<4.2.4 || >4.2.4,<5.0.0) requires importlib-metadata (>=5.2).
And because tox (4.2.3) depends on importlib-metadata (>=5.2)
 and tox (4.2.4) depends on importlib-metadata (>=5.2), tox (>=4.1.3,<5.0.0) requires importlib-metadata (>=5.2).
Because no versions of singer-sdk match >0.16.0,<0.17.0
 and singer-sdk (0.16.0) depends on importlib-metadata (<5.0.0), singer-sdk (>=0.16.0,<0.17.0) requires importlib-metadata (<5.0.0).
Thus, singer-sdk (>=0.16.0,<0.17.0) is incompatible with tox (>=4.1.3,<5.0.0).
So, because test depends on both singer-sdk (^0.16.0) and tox (^4.1.3), version solving failed.

Code

No response

@cwegener cwegener added kind/Bug Something isn't working valuestream/SDK labels Jan 6, 2023
@edgarrmondragon
Copy link
Collaborator

Yeah, the generated pyproject.toml can't be locked, tested with Poetry 1.3.

A few options, not necessarily mutually exclusive:

  • Remove Tox as a dev dependency. Not ideal since poetry run tox is somewhat convenient for folks who don't use Tox in other projects, so they probably have not installed it globally (and may not want to) with a tool like pipx.
  • To avoid this in the future, we should have poetry lock run in CI as part of the template tests, to make sure dependencies are compatible. For example, copier allows tasks to run after the project is generated and one task could be poetry lock. Related: feat: migrate cookiecutter templates to copier #1271

Workaround

In the meantime, removing tox from the generated pyproject.toml should fix the issue and let users create the lock file.

@cwegener
Copy link
Author

cwegener commented Jan 8, 2023

Another workaround for me was to use hatch instead of poetry btw.

@edgarrmondragon
Copy link
Collaborator

Another workaround for me was to use hatch instead of poetry btw.

@cwegener awesome, so is hatch able to resolve the dependencies even with tox in the mix?

@cwegener
Copy link
Author

cwegener commented Jan 9, 2023

Another workaround for me was to use hatch instead of poetry btw.

@cwegener awesome, so is hatch able to resolve the dependencies even with tox in the mix?

Yes. It is.

I didn't use hatch to replace tox but to replace poetry and I left the tox dev dependency in the mix. hatch has no problem correctly resolving the tox dependencies and is not getting tripped up by the importlib-metadata dependency from singer-sdk.

sdk/pyproject.toml

Lines 49 to 50 in 41d4c6b

importlib-metadata = {version = "<5.0.0", markers = "python_version < \"3.8\""}
importlib-resources = {version = "5.10.2", markers = "python_version < \"3.9\""}

@WillDaSilva
Copy link
Member

Currently the following dev dependencies are added to pyproject.toml when creating a target:

pytest = "^7.2.0"
tox = "^4.1.3"
flake8 = "^3.9.2"
black = "^22.12.0"
pydocstyle = "^6.2.1"
mypy = "^0.991"
isort = "^5.11.4"
types-requests = "^2.28.11.7"

Unlike the tap template the target template does not create a tox.ini file or mypy.ini, so none of these dependencies are actually used. Because these dependencies may introduce conflicts, and they are not currently used, I propose we remove them now, and re-add them later (with checks to avoid conflicts) if we want to add tox.ini for targets.

Thoughts @edgarrmondragon? Since this prevents new users from following the target development guide, I'd consider this a high priority.

As for taps, we probably also want to remove tox at least until the conflict can be resolved.

@WillDaSilva WillDaSilva removed their assignment Jan 24, 2023
@edgarrmondragon
Copy link
Collaborator

@WillDaSilva I'm ok with removing tox as a dependency (even permanently) if we document running with it pipx or similar.

@WillDaSilva WillDaSilva self-assigned this Jan 24, 2023
WillDaSilva added a commit that referenced this issue Jan 24, 2023
The documentation has been updated to suggest it be installed using `pipx`.

Closes #1311
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/Bug Something isn't working valuestream/SDK
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants