-
Notifications
You must be signed in to change notification settings - Fork 9
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 wheels cli #20
Add wheels cli #20
Conversation
Thanks @joemarshall for making this workflow more usable! I'll try to review it soon. |
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 @joemarshall a few comments below but overall it looks quite nice!
pyodide_lock/spec.py
Outdated
) -> None: | ||
"""Add a list of wheel files to this pyodide-lock.json | ||
|
||
Args: |
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.
We generally use numpy style docstrings
pyodide_lock/spec.py
Outdated
version = split_name[1] | ||
python_tag = split_name[-3] | ||
split_name[-2] | ||
platform_tag = split_name[-1] |
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.
This can be done more reliably with https://packaging.pypa.io/en/stable/utils.html#packaging.utils.parse_wheel_filename
pyodide_lock/spec.py
Outdated
requirements = get_wheel_dependencies(wheel_file, package.name) | ||
for r in requirements: | ||
req_marker = r.marker | ||
req_name = canonicalize_name(r.name) |
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.
Same question as in https://github.com/pyodide/pyodide-lock/pull/18/files#r1332985361 Are you sure the name should be normalized? Last I checked it wasn't in pyodide-lock.json
and one install by package name usually not the normalized name. But I could be mistaked.
pyodide_lock/spec.py
Outdated
else: | ||
raise RuntimeError( | ||
f"Requirement {req_name} is not in this distribution." | ||
) |
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.
pyodide_lock/utils.py
Outdated
@@ -74,3 +87,54 @@ def _generate_package_hash(full_path: Path) -> str: | |||
while chunk := f.read(4096): | |||
sha256_hash.update(chunk) | |||
return sha256_hash.hexdigest() | |||
|
|||
|
|||
def get_wheel_dependencies(wheel_path: Path, pkg_name: str) -> list[Requirement]: |
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 think we can parse them with importlib.metadata so we don't have to bother checking it's correct for all edge cases.
I realized the PRs you did @joemarshall and @bollwyvl are on the same topic with a very similar API. So it would be nice to take some parts of each. I don't know how you prefer to proceed, I'm tempted to to merge #18 first (since it's smaller and does fewer things), and then sync it with this PR and use methods/functions added there here. Assuming you are OK with the proposed API. But as you prefer, I'm open to suggestions. Also if you have some review comments, since you were thinking about similar use cases :) The one thing that is a blocker for both IMO, is that currently, both normalize the package name (and the name of the dependencies). At some level, it might not matter since we likely normalize the user-provided name when installing. But I feel like it should be the same in pyodide, micropip and here (whichever convention is chosen). |
On that topic, there was also pyodide/pyodide#4005 as not using project names, makes it not possible to install such packages via pip (if Now I'm confused, maybe we should be normalizing package names in |
Hmm, yes I think normalizing package names in pyodide-lock.json would be a good idea to make things simple. micropip is currently saving the normalized names of packages because micropip doesn't know the real project name before downloading the wheel and see the metadata in it, while the convention in I think using normalized names everywhere will fix the old problem in micropip: it cannot handle packages that are included in Pyodide but have non-normalized names (e.g. pyodide-lock.json has So I am +1 for using a normalized name in pyodide-lock.json. |
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.
OK I megered #18. @joemarshall would you mind syncing with upstream/main
and seeing if you could use the methods/functions added there? Thanks!
@rth @bollwyvl So, this PR now uses much of the code from #18. I did some refactoring which probably requires an opinion from @bollwyvl also, because it makes the dependency handling more robust, by messing with some of the code from #18. In particular: I've made
Because of this, rather than create a I made There is also some handling done in Oh and I improved the handling of marker environment to make it use the version numbers etc. from the PyodideLockSpec object, and it now checks wheel abi etc. against that also. I think I ported pretty much all the tests from #18 or equivalent into the new |
Welp, that's the one I really want to be able to use downstream. But frankly, even after adding that initial method, I feel like the data models should probably be "dumb", and only busy themselves with validation... and potentially removing the IO-based methods altogether. Going further, having non-pydantic types at the borders e.g. accept
The thing would be a valid package spec, but it's the lockfile as a whole that might be invalid... and that's not even being validated right now in However, with pyodide/micropip#83, this isn't even really the case, as dependencies could be sourced from an index, really getting to the use case I want: As a project, be able to ship one locally-built wheel and make it autoimportable. If this means having to fake up a PyPI index for its non-pydodide-distributed dependencies, I can live with that. At any rate: having the wheel spec generation be as simple and accurate as possible, and having the lock spec handle the higher-order invariants seems like it will keep code simpler and more easily tested.
Yep, while changing the field name would be bad, this should have a
Extras are frankly pretty broken, upstream, as they don't constrain future solves, aren't validated by If this is gonna do the thing, then the spec needs to add |
Ah - I think we have slightly different but related use cases here - My use case is to build a wheel along with all dependencies (so I have a folder of wheels), and make all those wheels import nicely in pyodide. So for me, I make the assumption that:
Assuming I built with This means that all I need here is to add the correct wheels to the pyodide-lock so they import correctly. No need to create a dummy pypi or anything. The reason for doing the propagation of extras into the dependencies here is to mirror what happens when you call As an example I have a package (lets call it If I install it with pip, it installs In pyodide-lock, if I ignore extras as it would if I used the original from_wheel method, then I first import all my wheels into the pyodide-lock. I agree that extras are pretty rubbish, and I can see why pyodide-lock doesn't support them, which is why I think the hack to resolve installed extras at the time of making the pyodide-lock file makes sense. After all, it does mirror the behaviour on a normal python, that if you ever install a package with a dependency that has an extra, all packages that use that dependency also have the extra included. I don't think there's anything particularly wrong with making My preferred option actually is to keep |
Good point. I think you're absolutely right. I've moved the Oh and done those other review changes above. I think hopefully we're at a point that this provides the functionality both of us want? |
@rth Hopefully ready for merging now |
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 @joemarshall!
I've left several comments about code details. Overall, I think it looks good, but regarding the high-level design, I wasn't involved into the discussion deeply, so it would be nice to get an additional review from @rth.
@rth Done all the review changes above now. |
@bollwyvl Any chance we could get this merged so it can drop off my todo list? |
Sorry for the long wait, if no one else review this by the end of the week, I'll check back over the weekend and merge this. |
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 again @joemarshall!
Released v0.1.0a4 including this change. |
This adds a very basic cli so in a you can do
pyodide lockfile add-wheels *.whl
and it will add wheels to apyodide-lock.json
in the current folder and write it out topyodide-lock-new.json
. There's options for input and output json.What this means is that if you do:
pyodide build --with-dependencies
to create a load of wheel files, you can then dopyodide lockfile add-wheels dist/*.whl
and it will create you a lockfile including your new wheels, with dependencies between these wheels correctly marked.No tests or anything yet, but thought it was worth pushing so you can see it.