FIX Respect yanked flag #208
Conversation
There was a problem hiding this comment.
Thanks, @ryanking13! I have just a few comments, the code looks good to me. Again, thanks for the reminder on Discord.
|
|
||
| ### Fixed | ||
|
|
||
| - micropip now respects the `yanked` flag in the PyPI simple API. |
There was a problem hiding this comment.
| - micropip now respects the `yanked` flag in the PyPI simple API. | |
| - micropip now respects the `yanked` flag in the PyPI Simple API. |
| # PEP-592: | ||
| # yanked can be an arbitrary string (reason) or bool. | ||
| # any string is considered as True, so we convert it to bool. | ||
| yanked = bool(file.get("yanked", False)) | ||
|
|
There was a problem hiding this comment.
From reading the PEP, I think we should also add support for the yanked_reason key as well, which we can show as a warning message of sorts if someone tries to install a "yanked" package by requesting it directly using await micropip.install("mypackage==<yanked_version_here>"). My suggestion comes from the line: "Tools that process the simple repository API MAY surface this string to end users". As our users have historically felt the need for better error messages from micropip, supporting this would be beneficial, IMO.
| @@ -327,6 +328,8 @@ def find_wheel(metadata: ProjectInfo, req: Requirement) -> WheelInfo: | |||
| reverse=True, | |||
| ) | |||
|
|
|||
| yanked_versions: list[list[WheelInfo]] = [] | |||
There was a problem hiding this comment.
Should this be just list[WheelInfo], similar to wheels = list(releases[ver]) below? Creating a list from a list seems to be redundant here.
| yanked_versions: list[list[WheelInfo]] = [] | |
| yanked_versions: list[WheelInfo] = [] |
| wheels = list(releases[ver]) | ||
|
|
||
| # If the version is yanked, put it in the end of the candidate list. | ||
| # If we can't find a wheel that satisfies the requirement, | ||
| # install the yanked version as a last resort. | ||
| yanked = any(wheel.yanked for wheel in wheels) | ||
| if yanked: | ||
| yanked_versions.append(wheels) | ||
| continue |
There was a problem hiding this comment.
I think this control flow makes things a bit confusing for me, coupled with the fact that the variable defined above is named just wheels which is not specific about what type of wheel is being found. I understand that we are trying a two-pass approach by first looking for non-yanked wheels, and then searching within the yanked wheels if no non-yanked wheel in the first pass was found to be a suitable candidate for installation.
Instead of checking if a yanked wheel exists, we could create a smaller list using a list comprehension and manage its lifetime separately instead of branching out? i.e., using yanked_wheels = [wheel for wheel in wheels if wheel.yanked] and then passing that to _find_best_wheel() (which can return None if it receives None as an input).
| def test_yanked_version(): | ||
| from micropip._vendored.packaging.src.packaging.requirements import Requirement | ||
| from micropip.transaction import find_wheel | ||
|
|
||
| versions = ["0.0.1", "0.15.5", "0.9.1"] |
There was a problem hiding this comment.
I think we should add two more tests, one test for the case where one requests a yanked wheel explicitly (similar to the above point), and another case which checks if a warning was raised or not upon installing a yanked wheel (if you agree with my above suggestion, that is).
Resolve: #146
Updates the resolver to respect
yankedflag (PEP-592).If the version with the yanked flag set is found, it will be considered as a last resort, when all other versions are incompatible.