Better freeze of distributions installed from direct URL references #7612

sbidoul · 2020-01-19T12:55:08Z

In a nutshell, before it did this:

$ pip install git+https://github.com/pypa/packaging
....
$ pip freeze
packaging==20.1.dev0
pyparsing==2.4.6
six==1.14.0

Now it does this:

$ pip install git+https://github.com/pypa/packaging
....
$ pip freeze
packaging @ git+https://github.com/pypa/packaging@d49fdc500a7c057b71851847ff8d7cc92865bcf2
pyparsing==2.4.6
six==1.14.0

sbidoul · 2020-01-28T08:58:39Z

@pypa/pip-committers I have a question following #7650 (comment). Should pip freeze emit PEP 440 direct URL references requirements (name @ URL), or should it emit pip-style (URL#egg=name) requirements?

This PR currently emits PEP 440 style requirements.

pfmoore · 2020-01-28T09:47:30Z

IMO it should use PEP 440 (so that requirements files are less tied to pip-specific details, and are more standards based). But that does mean that we need to make sure we support PEP 440 for all the URL styles we need - and #7650 suggests we have some tidying up to do there.

sbidoul · 2020-01-28T10:01:38Z

Good. Converting pip URLs to PEP 440 works just fine, because URL parsing is done by the Link class and get_url_rev_and_auth (for VCS specific stuff) in both cases. The git+file issue in #7650 is due to an over-zealous check being done in packaging.

chrahunt · 2020-02-02T18:34:26Z

src/pip/_internal/models/direct_url.py

+def _get(d, expected_type, key, default=None):
+    # type: (Dict[str, Any], Type[T], str, Optional[T]) -> Optional[T]
+    """Get value from dictionary and verify expected type."""
+    if key not in d:


try: value = d[key] except KeyError: return default

is more Pythonic IMO.

Oh. Is it? I've used both over time and I now try to avoid exceptions for flow control.

chrahunt · 2020-02-02T18:39:13Z

src/pip/_internal/models/direct_url.py

+
+def _exactly_one_of(infos):
+    # type: (Iterable[Optional[InfoType]]) -> InfoType
+    infos = [info for info in infos if info is not None]


Suggested change

infos = [info for info in infos if info is not None]

infos = list(filter(None, infos))

filter(None, iterable) removes falsy entries, not just None. It’s probably not a problem here, but I prefer the current comprehension implementation for clarity.

I also find list comprehension more readable, as its more explicit. With filter I have to think for a fraction of a second what None means as a filter function.

chrahunt · 2020-02-02T18:39:40Z

src/pip/_internal/models/direct_url.py

+def _exactly_one_of(infos):
+    # type: (Iterable[Optional[InfoType]]) -> InfoType
+    infos = [info for info in infos if info is not None]
+    if len(infos) == 0:


Suggested change

if len(infos) == 0:

if not infos:

Would this be too magical?

iterator = (info for info in infos if info is not None) try: info = next(iterator) except StopIteration: # missing one if next(iterator, None): # more than one return info

Would this be too magical?

I like it, although I would use iterator = filter(None, infos) here.

Would this be too magical?

Yes it would be, IMO. That requires me quite more than a fraction of a second to understand what this does compared to the couple of ifs. Also I tend to avoid using exceptions for flow control purposes.

To avoid exceptions here you can always do next(iterator, None). But that doesn’t help with the magical aspect.

chrahunt · 2020-02-02T18:44:55Z

src/pip/_internal/models/direct_url.py

+            else None,
+            vcs_info=self.info.to_dict()
+            if isinstance(self.info, VcsInfo)
+            else None,


I would we keep a name class member on the individual types which holds the key name and just use that instead of these individual checks

chrahunt · 2020-02-02T18:47:49Z

src/pip/_internal/models/direct_url.py

+    requested_revision = None  # type: Optional[str]
+    commit_id = None  # type: str
+    resolved_revision = None  # type: Optional[str]
+    resolved_revision_type = None  # type: Optional[str]


For this and the other classes can we specify these in an __init__ so it's clearer that these are to be used as object members and not class members?

Done. TIL that mypy understands that.

chrahunt · 2020-02-02T19:27:45Z

src/pip/_internal/operations/install/wheel.py

@@ -598,6 +611,7 @@ def install_wheel(
    pycompile=True,  # type: bool
    warn_script_location=True,  # type: bool
    _temp_dir_for_testing=None,  # type: Optional[str]
+    direct_url=None,  # type: DirectUrl


Type should be Optional[DirectUrl] - strict-optional is disabled in this file otherwise it would've been caught.

chrahunt · 2020-02-02T19:32:48Z

src/pip/_internal/operations/install/wheel.py

+        )
+        with open(temp_direct_url_path, 'wb') as temp_direct_url_file:
+            temp_direct_url_file.write(direct_url.to_json().encode("utf-8"))
+        shutil.move(temp_direct_url_path, direct_url_path)


Can we make an issue to use a safer write-then-move approach like we do here? If we don't fix it everywhere in this module before this PR gets merged then I'd prefer to keep this as-is so that they can all be fixed at once.

Issue created: #7699

The corresponding issue now has an approved PR (#7929); so I think it'd be nice if we could adopt that approach -- it'd probably need that PR to merge but I don't think there's any reason it can't be merged within this week. :)

chrahunt · 2020-02-02T19:35:25Z

tests/unit/test_wheel.py

+    def test_std_install_with_direct_url(self, data, tmpdir):
+        self.prep(data, tmpdir)
+        direct_url = DirectUrl()
+        direct_url.url = "file:///home/user/archive.tgz"


At first glance it seems kind of random that we're setting a different url than we would actually expect. I assume this is to ensure that install_wheel isn't incorrectly using the wheel path argument? Can we put a comment to that effect?

I'll add a comment. direct_url.url is typically the original requested link, while wheelpath is the result of a local build.

chrahunt · 2020-02-02T19:39:18Z

src/pip/_internal/cache.py

+        """Returns a link to a cached item if it exists, otherwise returns the
+        passed link. Also returns a flag indicating if the cached link is
+        in the persistent cache (True), in the ephemeral cache (False),
+        or not in cache (None).


How would it look to return something enum-like instead?

src/pip/_internal/operations/freeze.py

pypa-bot · 2020-02-06T15:38:41Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

sbidoul · 2020-03-31T21:06:02Z

Rebased to resolve conflict following de populate_link move in #7801

Still ready for another review round. @xavfernandez maybe? @chrahunt it would be great if you could update your review, I think the changes your requested are done.

Addressed.

pradyunsg · 2020-04-01T13:19:56Z

Other than a minor comment (#7612 (comment)), I think this PR looks pretty good.

I do think it'd be a good idea to merge this in ASAP, and iterate on this as we go; instead of trying to implement rest of the optional / nuanced functionality as part of this PR.

src/pip/_internal/models/direct_url.py

pradyunsg · 2020-04-01T19:49:31Z

Wait, no. Uhhhh. I didn't intend to actually do the merge.

Hopefully "use both changes" was the correct thing here. Otherwise, apologies for the extra work @sbidoul. 🙈

Return whether the link was found in the persistent or ephemeral cache.

pass it to install_wheel via install

sbidoul · 2020-04-01T21:01:55Z

Thanks @xavfernandez for the review.
No worries, @pradyunsg, I rebased, and applied adjacent_temp_file to direct_url.json too.

pradyunsg · 2020-04-01T22:19:47Z

Wheeee! I got to do the green button clicky stuff! ^.^

pradyunsg · 2020-04-01T22:22:03Z

Many thanks and congratulations to @sbidoul! Thanks everyone who contributed to or reviewed this PR! ^>^

bmartinn · 2020-04-01T23:15:53Z

@sbidoul well done !!!
Guys this is a great feature we have been wanting for a while now... Thank you!
Can't wait to test the first RC release 🎉 🎊

sbidoul · 2020-04-02T06:57:48Z

Thanks for merging @pradyunsg. Big thank you to everyone who helped with this in one way or another!

@bmartinn Thank you for the kind words. Note pip does not do RC releases, but I encourage you to test now by installing the master branch with pip install -U "pip @ https://github.com/pypa/pip/tarball/master" (in an activated virtualenv).

sbidoul force-pushed the pip609-v2-sbi branch 8 times, most recently from 7a22e23 to b8afe21 Compare January 25, 2020 16:56

sbidoul mentioned this pull request Jan 25, 2020

pip freeze does not list repo url #609

Closed

sbidoul force-pushed the pip609-v2-sbi branch 2 times, most recently from 6480ecf to 1be708f Compare January 26, 2020 12:07

sbidoul marked this pull request as ready for review January 26, 2020 12:36

sbidoul force-pushed the pip609-v2-sbi branch 2 times, most recently from ea038be to 5f60f6f Compare January 27, 2020 10:59

sbidoul mentioned this pull request Jan 28, 2020

Error installing git+file VCS references #7650

Closed

sbidoul mentioned this pull request Jan 29, 2020

Freezing local tarball packages suggests requirement change that doesn't work #192

Open

sbidoul force-pushed the pip609-v2-sbi branch 3 times, most recently from 5c0402c to bdd6e58 Compare February 1, 2020 12:41

chrahunt previously requested changes Feb 5, 2020

View reviewed changes

sbidoul added C: freeze 'pip freeze' related PEP implementation Involves some PEP labels Feb 5, 2020

sbidoul force-pushed the pip609-v2-sbi branch from bdd6e58 to 24d20cd Compare February 5, 2020 16:02

uranusjr mentioned this pull request Feb 6, 2020

Replace pkg_resources with importlib_metadata #7413

Closed

sbidoul mentioned this pull request Feb 6, 2020

pip does not reinstall an already installed package from a different direct url requirement #7678

Closed

sbidoul force-pushed the pip609-v2-sbi branch from 24d20cd to e9dfe30 Compare February 6, 2020 15:38

xavfernandez approved these changes Apr 1, 2020

View reviewed changes

src/pip/_internal/models/direct_url.py Outdated Show resolved Hide resolved

sbidoul added 8 commits April 1, 2020 22:56

Add DirectUrl model, implementing PEP 610

6b7f4ce

Add helper to convert DirectUrl to PEP 440 direct reference

6f689f6

Add helper to create a DirectUrl from a Link

88582c2

Add helper to get DirectUrl metadata from Distrribution

bd4d52b

Add DirectUrl support to install_wheel

f779447

Add WheelCache method to inform which cache was used

94b7713

Return whether the link was found in the persistent or ephemeral cache.

Add direct_url support to InstallRequirement

a0ed759

pass it to install_wheel via install

Better freeze using direct_url.json

196706d

sbidoul force-pushed the pip609-v2-sbi branch from 9b6bf6b to 196706d Compare April 1, 2020 20:57

pradyunsg approved these changes Apr 1, 2020

View reviewed changes

pradyunsg merged commit 4b159c5 into pypa:master Apr 1, 2020

bmartinn mentioned this pull request Apr 1, 2020

Requirements.txt and git+ssh python packge urls allegroai/clearml#62

Closed

sbidoul deleted the pip609-v2-sbi branch April 2, 2020 06:51

rouge8 mentioned this pull request Apr 9, 2020

Re-install PEP 508 wheel dependencies when the version changes #6402

Closed

This was referenced Apr 17, 2020

Oh look! It's April again! (Release 20.1) #7951

Closed

Wheel cache link look up in the new resolver #8066

Merged

sbidoul mentioned this pull request Apr 27, 2020

Fix permissions on dist-info files created by adjacent_temp_file #8144

Merged

deveshks mentioned this pull request Apr 30, 2020

pip freeze does not show version for in-place installs #8174

Closed

lock bot added the auto-locked Outdated issues that have been locked by automation label May 5, 2020

lock bot locked as resolved and limited conversation to collaborators May 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better freeze of distributions installed from direct URL references #7612

Better freeze of distributions installed from direct URL references #7612

sbidoul commented Jan 19, 2020 •

edited

Loading

sbidoul commented Jan 28, 2020 •

edited

Loading

pfmoore commented Jan 28, 2020

sbidoul commented Jan 28, 2020

chrahunt Feb 2, 2020

sbidoul Feb 5, 2020

chrahunt Feb 2, 2020

uranusjr Feb 5, 2020

sbidoul Feb 5, 2020

chrahunt Feb 2, 2020

uranusjr Feb 5, 2020

pradyunsg Feb 5, 2020

sbidoul Feb 5, 2020

uranusjr Feb 5, 2020

chrahunt Feb 2, 2020

chrahunt Feb 2, 2020

sbidoul Feb 6, 2020

chrahunt Feb 2, 2020

chrahunt Feb 2, 2020

sbidoul Feb 5, 2020

pradyunsg Apr 1, 2020

chrahunt Feb 2, 2020

sbidoul Feb 5, 2020

chrahunt Feb 2, 2020

pypa-bot commented Feb 6, 2020

sbidoul commented Mar 31, 2020

pradyunsg commented Apr 1, 2020 •

edited

Loading

pradyunsg commented Apr 1, 2020

sbidoul commented Apr 1, 2020

pradyunsg commented Apr 1, 2020

pradyunsg commented Apr 1, 2020

bmartinn commented Apr 1, 2020

sbidoul commented Apr 2, 2020 •

edited

Loading

	infos = [info for info in infos if info is not None]
	infos = list(filter(None, infos))

Better freeze of distributions installed from direct URL references #7612

Better freeze of distributions installed from direct URL references #7612

Conversation

sbidoul commented Jan 19, 2020 • edited Loading

sbidoul commented Jan 28, 2020 • edited Loading

pfmoore commented Jan 28, 2020

sbidoul commented Jan 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pypa-bot commented Feb 6, 2020

sbidoul commented Mar 31, 2020

pradyunsg commented Apr 1, 2020 • edited Loading

pradyunsg commented Apr 1, 2020

sbidoul commented Apr 1, 2020

pradyunsg commented Apr 1, 2020

pradyunsg commented Apr 1, 2020

bmartinn commented Apr 1, 2020

sbidoul commented Apr 2, 2020 • edited Loading

sbidoul commented Jan 19, 2020 •

edited

Loading

sbidoul commented Jan 28, 2020 •

edited

Loading

pradyunsg commented Apr 1, 2020 •

edited

Loading

sbidoul commented Apr 2, 2020 •

edited

Loading