-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add a common class to store JSON / Simple API response data #71
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ryanking13 that would be very useful indeed!
A couple of comments. Ping me when it's ready for a final review.
for more information, see https://pre-commit.ci
@rth Okay, I think this is ready to be reviewed. I cached In terms of performance, For example, this is a result when I tested with from micropip.package_index import ProjectInfo
from pathlib import Path
from gzip import decompress
import json
import timeit
numpy_json = json.loads(decompress(Path("tests/test_data/pypi_response/numpy_json.json.gz").read_bytes()))
numpy_simple = json.loads(decompress(Path("tests/test_data/pypi_response/numpy_simple.json.gz").read_bytes()))
num_runs = 100
def run_json():
ProjectInfo.from_json_api(numpy_json)
def run_simple():
ProjectInfo.from_simple_api(numpy_simple)
print("Total number of files:", len(numpy_simple["files"]))
print(f"Parsing JSON API for {num_runs} times:", timeit.timeit(run_json, number=num_runs), "seconds")
print(f"Parsing Simple API for {num_runs} times:", timeit.timeit(run_simple, number=num_runs), "seconds")
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactoring @ryanking13 ! A couple of minor comments, otherwise LGTM.
if abi_incompatible: | ||
abis_string = ",".join({tag.abi for tag in tags}) | ||
raise ValueError( | ||
f"Wheel abi '{abis_string}' is not supported. Supported abis are 'abi3' and 'cp{version}'." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I though we were not supporting abi3? I think there was a discussion either in the cibuildwheel or some other wheel related MR that abi3 didn't make sense for us.
If this was copied from existing code fine but maybe we should open a follow up issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are planning to break ABI with every Python version, then abi3 is a bit pointless. I think we can support it and just check that the interpreter version matches exactly. So it's supposed to be that cp311-abi3
works with all Pythons <= 311 but instead we check that the interpreter is exactly cp311
. For tools like cibuildwheel that generate wheels, there's no reason to support abi3. But if someone uses a tool like maturin to generate an abi3 wheel we should be able to consume it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
micropip/package_index.py
Outdated
try: | ||
version = Version(version_str) | ||
if str(version) != version_str: | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to keep this? As long as there is a version it's fine no? And str(version) has a non negligible runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I think it is okay to remove it now.
That check was introduced at pyodide/pyodide#2752, but I think it cannot happen on wheel files (unless someone modifies the version manually).
) | ||
|
||
releases_compatible = { | ||
version: _compatible_wheels(files, version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to go faster (no need to do it, just an idea). We can probably also subclass Version to something that has a frozen __str__
and __hash__
so it doesn't need to be re-computed at each dict comparison..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened an issue (#73) so that we can do it as a follow-up. I also think it is would be a good issue for new contributors if there is other sprint / tutorial events.
Right, converting string <--> Version takes quite a time... I'm not sure how many people will have performance issues with this (especially given that network latency is a much larger part of it), but it's definitely something we can optimize for. By the way, pyinstrument is cool. Thanks for introducing an awesome tool :) |
Thanks for the reviews! |
This adds an intermediate class
ProjectInfo
(I can't think of a better name... open to other names) that parses both JSON and Simple API PyPI responses. micropip now takes thisProjectInfo
class instead of a raw PyPI JSON response when searching for packagesThis is a preparation to make micropip support both JSON and Simple API.
Related: #62