[WIP] Feature/pip install modern #602
Conversation
|
Differences between this and #599 This PR..
(1) seems like a good idea, though not 100% sure we'd want to require anything this close to bleeding-edge For (2), this doesn't have to be a breaking change I think. What do you think about the proposal in #599, of maintaining current behavior and adding a For (3), I thought we needed to update it? You found a way of using the still-old release? :O |
Wheels are far from being bleeding edge. They've been used for a couple of years now. Wheels are the standard since a year or 2.
If there are some people that don't look at release notes or even testing before updating, they live an extremely extreme life. Though, I know Allan likes to not break compatibility AFAIK. On my side, if we can avoid the breakage, why not. But if it's necessary, why not too. |
I was actually referring to pip-19. |
|
I see, thanks for the clarification. I don't see any problem with using pip >= 19. 19.0 was released in January. 19.1 just got released (like 11 hours ago). So >19.0,<19.1 isn't considered bleeding edge while 19.1+ could be considered bleeding edge, but normally pip is quite stable. In 4-5 years, I've rarely hit pip issues. If there is something to worry about, it's PEP-518, not |
|
I agree with assessment of current rez-pip limitations. Wheels seem the way
forward.
Backwards compatibility in rez is paramount. For this reason I am in favour
of supporting this new code path either via a `--use-wheel` option as
Marcus suggests; or, a separate cli altogether (rez-pip2 or rez-wheel
perhaps - leaning towards rez-pip2).
The reason I suggest a separate cli is because I'd be concerned about
ending up with a confusing mix of wheel-compatible and non-wheel-compatible
options available in the same tool (ie some options that make sense in
combo with --use-wheel and some that do not).
Ira if you're getting good results then I'm keen to see more.
Thx
A
…On Wed, Apr 24, 2019 at 11:57 PM JeanChristopheMorinRodeoFX < ***@***.***> wrote:
I see, thanks for the clarification. I don't see any problem with using
pip >= 19. 19.0 was released in January. 19.1 just got released (like 11
hours ago). So >19.0,<19.1 isn't considered bleeding edge while 19.1+ could
be considered bleeding edge, but normally pip is quite stable. In 4-5
years, I've rarely hit pip issues. If there is something to worry about,
it's PEP-518 <https://www.python.org/dev/peps/pep-0518/>, not pip>=19.0
nor PEP-517` IMO.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#602 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAMOUSXBFMDK2YRFKDZV4YTPSBRK3ANCNFSM4HHC5GKA>
.
|
|
Hi @lambdaclan , @motto, How does this and #599 relate? Are they competing implementations that we need to choose from? Would like to talk more about these PRs in reference to one another before moving forward. Thx |
|
Hello Allan, thank you very much for taking the time to look at the pull request. I have been using my changes above for quite some time without any issues and I am sure Marcus can attest to this since he has been doing the same. Although the PR from Marcus is based on this one we are doing things a bit differently such as me enforcing wheel installs. I have also updated distlib but I did not push those changes so as not to make the PR harder to read. I will do so before the final merge. The PR clearly states that a new version of distlib is required for wheel installs to work. Maybe I was overly ambitious in thinking that we can get rid of the old style installs since they are considered legacy now but since backwards compatibility is important then I can keep the original install method in place. As such, I am happy to comply with the suggested solution of adding a new separate cli that will only work with wheels instead of using a flag such as --use-wheels to avoid the issue of wheel compatible incompatible packages as you mention. Using a specific command for wheel installs will be immediately apparent that packages are installed using wheel packages . In doing so, It will also be easier to deprecate the old code base when the time comes rather than trying to keep the code from a flag option. @nerdvegas @mottosso I am willing to do the changes outlined above if it is OK with you so please let me know so that I can proceed. As I mentioned in another post, I do see some duplication of work happening and it is a shame. We need a way to track who is doing what to encourage people to work on other features/issues or promote collaboration between community members. This is really important in my opinion, what do you think would be a suitable solution? Maybe having a rez kanban board? I am happy to work with Marcus on this if he wants otherwise I will just carry on. Looking forward to your input. Thank you. Ira |
In short, #599, #595 and #596 is consolidated into #614 which is akin to #628, but different. I would recommend consolidating this into #628 as well, so we can have a look at both and determine what we like and dislike about each and ultimately consolidate each into one for final merge. Smorgasbord!
IMO this is a little over the top. A simple "Hey, I'm working on X" in the chat should suffice. The reason for these duplicate PRs isn't a lack of kanban, but rather:
And as a result, we've got some lovely exploration and features to choose between. :) |
I do not mind either way so I will leave it up to Allan to decide. We will still need to extract the wheel installation method to a new separate cli tool as outlined above and add some tests. I am happy to help
That would be nice but unfortunately I do not think that everyone publicly announces what they are working on furthermore a message on chat can easily missed by other members of the community and also not seen at all by people not part of the chatting group. Having a kanban is more official and lets everyone know what active contributors are working on. It will also allow us to work on features/issues that matter based on a roadmap. I am not in control so there is not much I can do but these are my thoughts. I dont want to know how many people have branches doing the same work as others because their studio needs it... I want to make clear that I do not mind collaborating on the same issue with you or anyone else but we do need a way to track everything. |
src/rez/pip.py
Outdated
| for installed_file in distribution.list_installed_files(allow_fail=True): | ||
| source_file = os.path.normpath(os.path.join(destpath, installed_file[0])) | ||
| for installed_file in distribution.list_installed_files(): | ||
| if 'bin' in installed_file[0]: |
There was a problem hiding this comment.
It's not obvious what's going on here, a comment would help. Also, I take it that this covers a case where installed_file[0] is present within a 'bin' dir; in this case, a substring search isn't robust enough, I would search for 'bin' in the list generated from str.split(os.sep). I gather that placement of said bin dir is important also, I'd check for that as well.
There was a problem hiding this comment.
I seems like I also commented this line in https://github.com/nerdvegas/rez/pull/628/files#r296459995. I'm always confused when the same change appears in two different PRs.
Here is the specification of the database of installed python distributions: https://www.python.org/dev/peps/pep-0376/#record. I will probably help to do a proper implementation that will handle all cases.
There was a problem hiding this comment.
I have been looking into ways to improve the readability of this. Since this is installed as a wheel distribution, all the installed files are listed in a CSV file called RECORD including the bin scripts as such there is no worry of missing any files from the list.
I am aware of the specification for the database generated pointed out by JC but there seems to be some inconsistencies when combining the wheel install with the target flag to set a directory. Specifically, the bin script paths in the RECORD file
are wrong.
Essentially it points the bin scripts to be two directories up instead of being in the base location.
Have a look at an example install of pydocstyle library:
The bin folder is right there along with the rest of source files but a quick look into the RECORD file, revelas that the bin location should be somewhere else...
This is why you see all that weird parsing and path combining taking place. I am trying to see if I am missing something but the rest of the path are correct starting right from the destpath location as base.
Using --prefix instead of --target will put the files in a site-packages directory and the scripts in a bin folder
This time the RECORD file points the bin scripts to be 3 directories up from base.
which is strangely correct but doing so will require to parse the source packages out of lib/python/site-packages..
I will keep looking into for a bit longer and see if I find anything.
There was a problem hiding this comment.
Relevant issue: pypa/pip#3934
There was a problem hiding this comment.
I am now moving the bin folder to where the RECORD file expects it to be ../../bin and everything is working fine. The checks have become very simple.
There was a problem hiding this comment.
Yeah I see what you mean, it's odd!
src/rez/pip.py
Outdated
| if is_exe(source_file): | ||
| if destination_file.startswith("%s%s" % (os.path.join("python", "bin"), os.path.sep)): | ||
| destination_file = os.path.join("bin", os.path.basename(source_file)) | ||
| _, _file = os.path.split(destination_file) |
There was a problem hiding this comment.
_file = os.path.basename(destination_file)
|
Hey @lambdaclan, I'm pretty happy to merge this very soon, I just had one suggestion regarding backwards compatibility. Since support for pep517 was added in pip-19, I think it'd be sufficient to check the pip version and use this new code in that case, rather than introducing a --use-wheel as suggested before. It feels like that may be a cleaner way to go about it, and will avoid the confusion of extra options. It seems to me that wheels are prevalent enough that we can go that route regardless. What do you think? I'll be back on this tomorrow, I'm happy to make those changes if we want to do that and if you haven't already. I know this is holding up #628 so I'm keen to get it merged. Cheers |
src/rez/pip.py
Outdated
|
|
||
| with make_package(name, packages_path, make_root=make_root) as pkg: | ||
| pkg.version = distribution.version | ||
| pkg.version = distribution.metadata.version |
There was a problem hiding this comment.
Note that we need to be careful with this. The python version schema specification isn't 100% compatible with rez.
There was a problem hiding this comment.
I have been using metadata in a couple of places for summary and name, those shouldn't be an issue I guess. I never had an issue with version but I can revert back to the original if necessary.
This is an issue I have been getting but I fixed it on the other PR
There was a problem hiding this comment.
I'd second JC here, even if initially there's no remapping at all, there should be a separate pip_to_rez_version function.
Taking a look at https://www.python.org/dev/peps/pep-0440/#version-scheme though, there are some things to take into account. I can see two potential issues
1: version epochs (they make no sense to rez, so they'd just get stripped of the leading N!;
2: python versions are case insensitive, so they should probably be lowercased when converted to a rez version.
There was a problem hiding this comment.
Local versions are also not compatible with rez (https://www.python.org/dev/peps/pep-0440/#local-version-segments) BTW, local is something we use at RodeoFX for our pip packages when we fork a repo for example. Though, we don't use rez-pip since we have our own implementation. But some other studios could use the local versions. At Rodeo, we basically convert package-1.0.0+<something> to package-1.0.0-<something>.
Most possibilities are described in https://www.python.org/dev/peps/pep-0440/#examples-of-compliant-version-schemes.
|
Hello @nerdvegas and @JeanChristopheMorinRodeoFX Apologies for the delayed reply. With the latest progress taking place in the repository it is becoming diffucult to keep track of all the incoming emails. I was waiting to see what will happen between this PR and Marcuse's one from #614
Since you have decided to move forwards with this PR and #628 then I will go ahead and the do the changes requested. If anything else pops up please comment on either PR and I will look into it. I will keep an eye on both to make sure I do not miss anything. |
Remove staging directory paths required by old pip install method. Add comment to explain why the bin path although unused has been retained. Relates pypa/pip#5983, #475, #503
Update the pip install arguments to use a specific target directory to install the requested library. Set the install command to use PEP 517 for building source distributions to ensure a wheel is always built before installation. Relates pypa/pip#4501, #475, #503
Use metadata attributes to retrieve core data such as the name, version and summary of the requested package. Relates https://www.python.org/dev/peps/pep-0426/#core-metadata, #475, #503
Use wheel dist-info metadata and record files to perform the installation of the requested package including all its dependencies. BREAKING CHANGE: Currently rez is using `distlib` 0.2.4 released in late 2016. That version of the library does not correctly detect installed wheel distributions meaning that there is no access to metadata and record files. To fix your project, update your `distlib` library located in the src/vendor directory.
Revert dependency gathering method because new API methods such as distutils DependencyFinder and metadata extra, dependency fields are still not well documented or widely adopted with package maintainers. Ensure each dependency is installed as an individual rez package to avoid duplicates. Relates nerdvegas#475, nerdvegas#503
|
Thanks Ira! Both rez-pip and the installation process are things I want to
get sorted asap, so I'll be as responsive as possible on this.
Just a note - my family and I are moving Friday this week so I'll more than
likely go dark for 3-4 days, so if I don't reply to emails around then,
that's why.
Cheers
A
…On Tue, Jun 25, 2019 at 9:46 AM Ira ***@***.***> wrote:
Hello @nerdvegas <https://github.com/nerdvegas> and
@JeanChristopheMorinRodeoFX
<https://github.com/JeanChristopheMorinRodeoFX>
Apologies for the delayed reply. With the latest progress taking place in
the repository it is becoming diffucult to keep track of all the incoming
emails.
I was waiting to see what will happen between this PR and Marcuse's one
from #614 <#614>
I am happy to help if Marcus is on it then I will do something else.
Sure, I'm happy to handle pip. Each of yours and Allans requirements (both
here and #628 <#628>) are included
in #614 <#614> so we could continue
from there.
Since you have decided to move forwards with this PR and #628
<#628> then I will go ahead and the
do the changes requested.
If anything else pops up please comment on either PR and I will look into
it. I will keep an eye on both to make sure I do not miss anything.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#602>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMOUST26YVXOTF5AISRN5DP4FMHHANCNFSM4HHC5GKA>
.
|
|
Hello Allan, no problem at all, thanks for letting us know. I hope the move goes smoothly! I am going through your and JC comments now and should have something ready for review fairly soon. Ira |
|
Thanks for letting us know @nerdvegas . @lambdaclan I'll keep an eye open and will do another pass of review when you will push. You might have to look at #628 because I might have do some comments there that are applicable here (sorry about that). Just so we are all on the same page, this PR enforces the usage of wheels only and I think we will need a PR to update distlib right? |
|
Hello JC, thank you very much for your time reviewing my PRs. Feedback from you and Allan is highly appreciated since I am fairly new to the project and want to make sure that everything works as intended without comprising existing functionality. Indeed, this PR enforces the usage of wheels, if a packages does not have a wheel one is automatically built during install and then process continues as expected. As pip updates keep coming in we will possibly need to make adjustments to rez-pip as well. For example enforcing pep-517 might become the default in the near future. In terms of distlib, it is a pre-requisite for this feature (and #628) to work so I can either make another PR or add it to this one. I purposely kept it out to make it easier to read. PR #628 depends on this one since wheel installation method is a per-requisite. Let us get this one sorted out first and then I will move onto that one. Hope this clears things a bit. Ira |
|
Hey @lambdaclan, It seems like retaining backwards compatibility for pip-<19 may be more effort than it's worth. I think it'd be fine to move ahead without out it, so long as there's a check for this which prints an obvious error message. Let's just go with that? |
|
Um, that close was an accident!! |
src/rez/pip.py
Outdated
| distributions = [d for d in distribution_path.get_distributions()] | ||
|
|
||
| folders = [folder for folder in os.listdir(destpath) if os.path.isdir(os.path.join(destpath, folder))] |
There was a problem hiding this comment.
I would add a comment along the lines of, "moving bin folder to expected relative location, rather than under python/" etc. Also, simpler code would suffice:
staged_binpath = os.path.join(destpath, "bin")
if os.path.isdir(staged_binpath):
shutil.move(...)
There was a problem hiding this comment.
Yes indeed, since we only care about that path no reason to loop. It always seems to be under the same location so no need to recurse the directory for other bin folders. That is what I had in mind initially.
src/rez/pip.py
Outdated
| # destpath = /tmp/pip-HBkRGa-rez/re]z_staging/python | ||
| # installed_file[0] = ../../bin/pydocstyle | ||
| # normpath of /tmp/pip-HBkRGa-rez/rez_staging/python../../bin/pydocstyle = /tmp/pip-HBkRGa-rez/rez_staging/bin/pydocstyle OK | ||
| # normapth of /tmp/pip-HBkRGa-rez/rez_staging/python/../../bin/pydocstyle = /tmp/pip-HBkRGa-rez/bin/pydocstyle WRONG |
There was a problem hiding this comment.
The comment and implementation threw me for a little bit here.
The comment should just explain the odd case: that ../../bin/etc is provided by distlib when in fact ../bin/etc seems to be the correct path instead. I was a little confused because the comment here delves into the implementation more than it does explain the situation.
There was a problem hiding this comment.
Agreed, I was overly verbose aplogies. I will remove the implementation details and reword the comment a bit.
src/rez/pip.py
Outdated
| # installed_file[0] = ../../bin/pydocstyle | ||
| # normpath of /tmp/pip-HBkRGa-rez/rez_staging/python../../bin/pydocstyle = /tmp/pip-HBkRGa-rez/rez_staging/bin/pydocstyle OK | ||
| # normapth of /tmp/pip-HBkRGa-rez/rez_staging/python/../../bin/pydocstyle = /tmp/pip-HBkRGa-rez/bin/pydocstyle WRONG | ||
| if installed_file[0].startswith("."): |
There was a problem hiding this comment.
I would make this clearer, perhaps like so:
installed_filepath = installed_file[0]
bin_prefix = os.sep.join('..', '..', 'bin') + os.sep
...
if installed_filepath.startswith(bin_prefix):
# account for extra parentdir as explained above
installed = os.path.join(destpath, '_', installed_filepath)
else:
...
There was a problem hiding this comment.
Never knew that normpath will normalize a pathname by collapsing all redundant separators and up-level references. I always thought it was limited to dots and slashes, awesome. 👍
src/rez/pip.py
Outdated
|
|
||
| if os.path.exists(source_file): | ||
| destination_file = installed_file[0].split(stagingsep)[1] | ||
| destination_file = source_file.split(stagingsep)[1] |
There was a problem hiding this comment.
I apologise for my own (old) rubbish code! This split is odd, simple use of os.path.relpath would be clearer
destination_file = os.path.relpath(source_file, stagingdir)
src/rez/pip.py
Outdated
| @@ -317,11 +332,10 @@ def make_root(variant, path): | |||
|
|
|||
| variant_reqs.append("python-%s" % py_ver) | |||
|
|
|||
| name, _ = parse_name_and_version(distribution.name_and_version) | |||
| name = distribution.name[0:len(name)].replace("-", "_") | |||
| name = distribution.metadata.name | |||
There was a problem hiding this comment.
The rez package name can't be simply set to the dist name, because some pip packages have hyphen in the name. In rez this is not a valid package name (it would be interpreted as the start of the version, eg my-pkg-1.2 is 'my', version 'pkg-1.2').
In fact my own original code wasn't clear enough - even if all it's doing is replacing hyphen with underscore (as the original code did), it would be clearer if it were in a func such as pip_to_rez_package_name. This behaviour also needs to stay, for backwards compatibility reasons (if a studio used new rez-pip which then mapped to a different rez pkg name, that would cause a lot of problems).
|
Thanks @lambdaclan, please see comments. I think we're close. I'll merge the distlib version update PR along with this once it's ready. FYI tomorrow night is my last chance to do the merge before I move house! My pc will be packed up after that :) Cheers all |
|
@lambdaclan Have you tested with both a package that is only available as a source distribution and also tested with a wheel and an egg (Maybe we can skip eggs, as this is quite an old format that I've not seen in a while in PyPI)? Thinking quickly, |
|
Hello @nerdvegas
Sounds good to me, definitely keeping the old codebase in for installations from source would take some time, with wheels available for almost every package a pip version check as you said, should suffice. Furthermore, when a wheel is not available one is being built automatically.
Hello JC, I have been using this for quite sometime and did not have any issues, the issue I has I mentioned before was a Schema error --> In the PR post I mentioned the following:
Saying that, I am happy to try more packages that might give issues such as the ones you mention so I will try them both (PyYAML and psutil) and let you know how it goes.
That is great Allan, lets try to get this merged today if everything goes smoothly. I will start working on the changes necessary as per your comments right away. In terms of the metadata version name, summary etc, I revert back to the orginal function for getting those now that I am aware of the issues. Ira |
rez will now enforce wheel distribution based installations. A version of pip >= 19 is required in order to do so. Add pip version check to the callbacks of rez-pip command to validate that an up to date pip is being used. Relates: #602 #628 https://pip.pypa.io/en/stable/news/#id49
|
Tests: pip < 19
PyYAML
psutil
Review round 3. Only thing left is the version conversion I believe. Can you provide an example of a version that will not work via distribution.version? I can seem to find any libraries with exotic versions to try... Something like 1!1.0RC1 (epoch plus case insensitive?) and what it should turn into. I am asure we can use this to extract and convert it to a rez compatible version --> https://www.python.org/dev/peps/pep-0440/#appendix-b-parsing-version-strings-with-regular-expressions |
|
Using the REGEX mentioned above, we can easilly retrieve all the parts of a pip version and modify it accordingly.
..... within the pip_to_rez_version function parse segments and construct rez-compatible version Sample output: What I need to know is some sort of a ruleset, for example if only release key is set then keep as it is if it has epoch etc then remove it etc. Examplesversion with epoch --> 1!1.0 ---> rez compatible version: 1.0 Edit: Went ahead and implemented a rough approach of the above, I am sure changes will be required upon review but at least we have something to work with. |
|
Looks good, only some minor changes. My testing worked, let's deal with any further issues in separate PR. Thanks for the hard work! |
|
Wow, that was fast! Sounds good Allan. Please let me know of anything that needs to be changed and I will take care of it. I will focus on #628 next. |
Description
[In collaboration with #599]
The Python community is steadily moving away from eggs and transitioning into wheels.
As per the Python wheels website
One of the most important differences is the cross platform compatibility. Furthermore the current
implementation of the pip install command is using install options to split and point the installation to a custom directory (libs, scripts, includes etc) is enforcing the use of source packages and disables the
modern pip default of using a wheel if available. This means that everything that is not a pure python package needs a proper, working and matching compiler setup making it a real pain for Windows users whom may need multiple visual studios versions etc...
The difference with #599 is that it enforces the use of wheels via PEP 517 which seems to be the way forward. If a package does not have a wheel then one is being built prior to installation ensuring
consistency.
There is still ongoing discussion here from the pip maintainers but a few things that got my attention are the following statements:
Another interesting discussion to follow is the one here about handling install options with wheel packages.
Relates # (issue)
rez #475, #503 maybe more...
pip pypa/pip#4611 pypa/pip#4501
Type of change
Remarks
How Has This Been Tested?
I have installed a plethora of packages from known to obscure with good success rate.
Some issues still occur (unable to build a wheel - very rare) depending on the platform but the same problems would also occur with the current implementation.
I have also successfully installed the majority of the packages listed on the Python wheels website which do not yet provide a wheel without issues.
Test Configuration:
Todos
Checklist: