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

Ensure consistent extras formatting in output #2013

Merged
merged 4 commits into from
May 13, 2024

Conversation

AndydeCleyre
Copy link
Contributor

@AndydeCleyre AndydeCleyre commented Nov 2, 2023

This aims to resolve the discussion in #2004, and is a WIP (all contributions or alternatives are welcome).

_compat utils now exports install_req_from_line and canonicalize_ireq for other modules to use. This uses our copy_install_requirement which is updated to canonicalize extra strings.

As of now, canonicalize_ireq only affects the extras attributes, as that's all we need. So it may be poorly named 🤷🏼

No tests yet.

To avoid circular imports, this relocates PIP_VERSION from _compat to utils, as IMO utils should be importable by as many other piptools modules as possible.

Contributor checklist
  • Included tests for the changes.
  • PR title is short, clear, and ready to be included in the user-facing changelog.
Maintainer checklist
  • Verified one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@AndydeCleyre AndydeCleyre added bug Something is not working good first issue A good item for first time contributors to work on extras Handling optional dependencies labels Nov 2, 2023
@webknjaz webknjaz linked an issue Nov 2, 2023 that may be closed by this pull request
@AndydeCleyre AndydeCleyre force-pushed the bugfix/2004 branch 3 times, most recently from eb5215d to 4ffd903 Compare November 2, 2023 22:47
@AndydeCleyre

This comment was marked as resolved.

ireq, extras=set(map(canonicalize_name, ireq.extras))
)
if ireq.req:
ireq.req.extras = set(ireq.extras)
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean

Suggested change
ireq.req.extras = set(ireq.extras)
ireq.req.extras = set(map(canonicalize_name, ireq.extras))

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because they get normalized on line 101.

@webknjaz
Copy link
Member

webknjaz commented Nov 3, 2023

copy_install_requirement

No preference here.

@AndydeCleyre
Copy link
Contributor Author

copy_install_requirement

No preference here.

@atugushev Do you think this logic belongs in or out of copy_install_requirement?

Comment on lines 100 to 101
ireq = copy_install_requirement(
ireq, extras=set(map(canonicalize_name, ireq.extras))
Copy link
Member

@atugushev atugushev Nov 4, 2023

Choose a reason for hiding this comment

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

Do you think this logic belongs in or out of copy_install_requirement?

That sounds reasonable. Let's move this normalization to copy_install_requirement and use this function in all our InstallRequirement constructors.



def install_req_from_line(*args: Any, **kwargs: Any) -> InstallRequirement:
return canonicalize_ireq(_install_req_from_line(*args, **kwargs))
Copy link
Member

Choose a reason for hiding this comment

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

It might be worthwhile to normalize extras that could be passed to pip-compile --extra as well. What do you think @AndydeCleyre?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to first see the current display of the extras in that case, but I haven't been able to get those extras-names to show up at all in the output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not inclined to do it unless someone can demonstrate how or if it would make any difference.

@AndydeCleyre AndydeCleyre changed the title Ensure canonicalization of extras Ensure consistent extras formatting in output Nov 5, 2023
@AndydeCleyre AndydeCleyre marked this pull request as ready for review November 5, 2023 16:18
@webknjaz webknjaz self-requested a review December 3, 2023 03:26
@AndydeCleyre AndydeCleyre force-pushed the bugfix/2004 branch 3 times, most recently from 4612ba7 to 7711ce6 Compare February 22, 2024 13:50
@AndydeCleyre

This comment was marked as resolved.

@tboddyspargo

This comment was marked as resolved.

@AndydeCleyre
Copy link
Contributor Author

AndydeCleyre commented Feb 26, 2024

OK, here's a problem I could use help with:

reqs.in:

sentry_sdk[pure_eval]
$ pip-compile --no-header reqs.in
certifi==2024.2.2
    # via sentry-sdk
sentry-sdk[pure-eval]==1.40.5
    # via -r reqs.in
urllib3==2.2.1
    # via sentry-sdk
$ pip-sync /dev/null
$ pip install --dry-run --quiet --report=- -r reqs.in | jq -r '.install[].metadata.name'
urllib3
asttokens
certifi
executing
pure-eval
sentry-sdk
six

I'm demoting this to WIP again until this gets resolved.


In its current state, this branch has resolver_result missing items:

--- pip-tools-main.txt	2024-02-26 14:40:10.004827841 -0500
+++ pip-tools-2013.txt	2024-02-26 14:39:52.064453749 -0500
@@ -3,19 +3,12 @@
         'sentry-sdk[pure-eval]': ExtrasCandidate(base=LinkCandidate('https://files.pythonhosted.org/packages/f6/21/e41b8dd0da432a06b8bf10b7ba634d6e62e60b9a79aba15146a2581265cc/sentry_sdk-1.40.5-py2.py3-none-any.whl (from https://pypi.org/simple/sentry-sdk/)'),
         extras=frozenset({'pure-eval'})),
         'urllib3': LinkCandidate('https://files.pythonhosted.org/packages/a2/73/a68704750a7679d0b6d3ad7aa8d4da8e14e151ae82e6fee774e6e0d05ec8/urllib3-2.2.1-py3-none-any.whl (from https://pypi.org/simple/urllib3/) (requires-python:>=3.8)'),
-        '<Python from Requires-Python>': <pip._internal.resolution.resolvelib.candidates.RequiresPythonCandidate object at 0x75b8142a9af0>,
-        'asttokens': LinkCandidate('https://files.pythonhosted.org/packages/45/86/4736ac618d82a20d87d2f92ae19441ebc7ac9e7a581d7e58bbe79233b24a/asttokens-2.4.1-py2.py3-none-any.whl (from https://pypi.org/simple/asttokens/)'),
+        '<Python from Requires-Python>': <pip._internal.resolution.resolvelib.candidates.RequiresPythonCandidate object at 0x743e650b7b60>,
         'certifi': LinkCandidate('https://files.pythonhosted.org/packages/ba/06/a07f096c664aeb9f01624f858c3add0a4e913d6c96257acb4fce61e7de14/certifi-2024.2.2-py3-none-any.whl (from https://pypi.org/simple/certifi/) (requires-python:>=3.6)'),
-        'executing': LinkCandidate('https://files.pythonhosted.org/packages/80/03/6ea8b1b2a5ab40a7a60dc464d3daa7aa546e0a74d74a9f8ff551ea7905db/executing-2.0.1-py2.py3-none-any.whl (from https://pypi.org/simple/executing/) (requires-python:>=3.5)'),
-        'pure-eval': LinkCandidate('https://files.pythonhosted.org/packages/2b/27/77f9d5684e6bce929f5cfe18d6cfbe5133013c06cb2fbf5933670e60761d/pure_eval-0.2.2-py3-none-any.whl (from https://pypi.org/simple/pure-eval/)'),
-        'sentry-sdk': LinkCandidate('https://files.pythonhosted.org/packages/f6/21/e41b8dd0da432a06b8bf10b7ba634d6e62e60b9a79aba15146a2581265cc/sentry_sdk-1.40.5-py2.py3-none-any.whl (from https://pypi.org/simple/sentry-sdk/)'),
-        'six': LinkCandidate('https://files.pythonhosted.org/packages/d9/5a/e7c31adbe875f2abbb91bd84cf2dc52d792b5a01506781dbcf25c91daf11/six-1.16.0-py2.py3-none-any.whl (from https://pypi.org/simple/six/) (requires-python:>=2.7,
-        !=3.0.*,
-        !=3.1.*,
-        !=3.2.*)')
+        'sentry-sdk': LinkCandidate('https://files.pythonhosted.org/packages/f6/21/e41b8dd0da432a06b8bf10b7ba634d6e62e60b9a79aba15146a2581265cc/sentry_sdk-1.40.5-py2.py3-none-any.whl (from https://pypi.org/simple/sentry-sdk/)')
     },
-    graph=<pip._vendor.resolvelib.structs.DirectedGraph object at 0x75b8143a0a10>,
-    criteria={'sentry-sdk[pure-eval]': Criterion((SpecifierRequirement('sentry_sdk[pure_eval]'),
+    graph=<pip._vendor.resolvelib.structs.DirectedGraph object at 0x743e658c9af0>,
+    criteria={'sentry-sdk[pure-eval]': Criterion((SpecifierRequirement('sentry_sdk[pure-eval]'),
     via=None)),
     'sentry-sdk': Criterion((ExplicitRequirement(LinkCandidate('https://files.pythonhosted.org/packages/f6/21/e41b8dd0da432a06b8bf10b7ba634d6e62e60b9a79aba15146a2581265cc/sentry_sdk-1.40.5-py2.py3-none-any.whl (from https://pypi.org/simple/sentry-sdk/)')),
     via=ExtrasCandidate(base=LinkCandidate('https://files.pythonhosted.org/packages/f6/21/e41b8dd0da432a06b8bf10b7ba634d6e62e60b9a79aba15146a2581265cc/sentry_sdk-1.40.5-py2.py3-none-any.whl (from https://pypi.org/simple/sentry-sdk/)'),
@@ -30,26 +23,8 @@
     extras=frozenset({'pure-eval'}))),
     (SpecifierRequirement('urllib3>=1.26.11; python_version >= "3.6"'),
     via=LinkCandidate('https://files.pythonhosted.org/packages/f6/21/e41b8dd0da432a06b8bf10b7ba634d6e62e60b9a79aba15146a2581265cc/sentry_sdk-1.40.5-py2.py3-none-any.whl (from https://pypi.org/simple/sentry-sdk/)'))),
-    'pure-eval': Criterion((SpecifierRequirement('pure-eval; extra == "pure_eval"'),
-    via=ExtrasCandidate(base=LinkCandidate('https://files.pythonhosted.org/packages/f6/21/e41b8dd0da432a06b8bf10b7ba634d6e62e60b9a79aba15146a2581265cc/sentry_sdk-1.40.5-py2.py3-none-any.whl (from https://pypi.org/simple/sentry-sdk/)'),
-    extras=frozenset({'pure-eval'})))),
-    'executing': Criterion((SpecifierRequirement('executing; extra == "pure_eval"'),
-    via=ExtrasCandidate(base=LinkCandidate('https://files.pythonhosted.org/packages/f6/21/e41b8dd0da432a06b8bf10b7ba634d6e62e60b9a79aba15146a2581265cc/sentry_sdk-1.40.5-py2.py3-none-any.whl (from https://pypi.org/simple/sentry-sdk/)'),
-    extras=frozenset({'pure-eval'})))),
-    'asttokens': Criterion((SpecifierRequirement('asttokens; extra == "pure_eval"'),
-    via=ExtrasCandidate(base=LinkCandidate('https://files.pythonhosted.org/packages/f6/21/e41b8dd0da432a06b8bf10b7ba634d6e62e60b9a79aba15146a2581265cc/sentry_sdk-1.40.5-py2.py3-none-any.whl (from https://pypi.org/simple/sentry-sdk/)'),
-    extras=frozenset({'pure-eval'})))),
     '<Python from Requires-Python>': Criterion((RequiresPythonRequirement('>=3.8'),
     via=LinkCandidate('https://files.pythonhosted.org/packages/a2/73/a68704750a7679d0b6d3ad7aa8d4da8e14e151ae82e6fee774e6e0d05ec8/urllib3-2.2.1-py3-none-any.whl (from https://pypi.org/simple/urllib3/) (requires-python:>=3.8)')),
     (RequiresPythonRequirement('>=3.6'),
-    via=LinkCandidate('https://files.pythonhosted.org/packages/ba/06/a07f096c664aeb9f01624f858c3add0a4e913d6c96257acb4fce61e7de14/certifi-2024.2.2-py3-none-any.whl (from https://pypi.org/simple/certifi/) (requires-python:>=3.6)')),
-    (RequiresPythonRequirement('>=3.5'),
-    via=LinkCandidate('https://files.pythonhosted.org/packages/80/03/6ea8b1b2a5ab40a7a60dc464d3daa7aa546e0a74d74a9f8ff551ea7905db/executing-2.0.1-py2.py3-none-any.whl (from https://pypi.org/simple/executing/) (requires-python:>=3.5)')),
-    (RequiresPythonRequirement('!=3.0.*,!=3.1.*,!=3.2.*,>=2.7'),
-    via=LinkCandidate('https://files.pythonhosted.org/packages/d9/5a/e7c31adbe875f2abbb91bd84cf2dc52d792b5a01506781dbcf25c91daf11/six-1.16.0-py2.py3-none-any.whl (from https://pypi.org/simple/six/) (requires-python:>=2.7,
-    !=3.0.*,
-    !=3.1.*,
-    !=3.2.*)'))),
-    'six': Criterion((SpecifierRequirement('six>=1.12.0'),
-    via=LinkCandidate('https://files.pythonhosted.org/packages/45/86/4736ac618d82a20d87d2f92ae19441ebc7ac9e7a581d7e58bbe79233b24a/asttokens-2.4.1-py2.py3-none-any.whl (from https://pypi.org/simple/asttokens/)')))
+    via=LinkCandidate('https://files.pythonhosted.org/packages/ba/06/a07f096c664aeb9f01624f858c3add0a4e913d6c96257acb4fce61e7de14/certifi-2024.2.2-py3-none-any.whl (from https://pypi.org/simple/certifi/) (requires-python:>=3.6)')))
 })

@AndydeCleyre AndydeCleyre marked this pull request as draft February 26, 2024 19:03
@AndydeCleyre
Copy link
Contributor Author

I thought that pip was handling "canonicalized" extras, but:

$ pip-sync /dev/null
$ pip install --dry-run --quiet --report=- 'sentry_sdk[pure_eval]' | jq -r '.install[].metadata.name'
urllib3
asttokens
certifi
executing
pure-eval
sentry-sdk
six
$ pip install --dry-run --quiet --report=- 'sentry_sdk[pure-eval]' | jq -r '.install[].metadata.name'
urllib3
certifi
sentry-sdk

So I need to step back and request a sanity check about what we should be doing here. @atugushev ?

@webknjaz
Copy link
Member

Sorry, this fell off of my radar. Sounds like a bug in pip to fix as well?

@AndydeCleyre
Copy link
Contributor Author

Sounds like a bug in pip to fix as well?

Maybe these:

@AndydeCleyre
Copy link
Contributor Author

So I'll take this back out of draft but maybe we should consider it blocked until pip is fixed.

@AndydeCleyre
Copy link
Contributor Author

AndydeCleyre commented May 4, 2024

Sounds like a bug in pip to fix as well?

Maybe these:

These were just marked as resolved! I haven't retested with pip from git yet but I'm optimistic.


EDIT: Yes, this seems to be working fine with pip's main branch.

In fact it might be proper to undo some of these changes, if we can require the next pip release as a minimum. I'll add a commit undoing changes as long as the test still passes and see what everyone thinks/tests.

Copy link
Contributor

@chrysle chrysle left a comment

Choose a reason for hiding this comment

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

This looks good to me. I think we should move it forward? We can still respond to any issues this might cause afterwards (though I'm optimistic).

@chrysle
Copy link
Contributor

chrysle commented May 10, 2024

CI is fixed on main now. Could you rebase?

@AndydeCleyre
Copy link
Contributor Author

I'm getting a failure with test_diff_should_not_uninstall, but on main as well.

@AndydeCleyre
Copy link
Contributor Author

I rebased onto main, but it looks like the recently merged PR #2087 introduced flake8 and mypy violations in test_pip_compat.py and pip_compat.py, so those are now replicated here.

@chrysle
Copy link
Contributor

chrysle commented May 11, 2024

Hmm yes, I also wondered whether the one or other import was necessary, but trusted that pre-commit would catch that. I think that check should be marked as required to pass before merging, seems like it isn't.

@chrysle chrysle added this to the 7.4.2 milestone May 11, 2024
@chrysle
Copy link
Contributor

chrysle commented May 11, 2024

The linters ought to be satisfied, now.

@AndydeCleyre
Copy link
Contributor Author

Thanks. I won't have access to a computer for a day but feel free to rebase again.

@chrysle
Copy link
Contributor

chrysle commented May 11, 2024

I'll wait for you. Happy time out!

_compat now exports install_req_from_line and canonicalize_ireq
canonicalize_ireq now returns a copy ireq instead of modifying in place
@chrysle chrysle added this pull request to the merge queue May 13, 2024
Merged via the queue into jazzband:main with commit 5330964 May 13, 2024
34 checks passed
@chrysle
Copy link
Contributor

chrysle commented May 13, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working extras Handling optional dependencies good first issue A good item for first time contributors to work on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pip==23.3 breaking "extras" behavior
5 participants