Skip to content
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 support for constraints, install wheels before pyodide packages #181

Merged
merged 22 commits into from
Feb 1, 2025

Conversation

bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Jan 27, 2025

references

changes

breaking changes

Sorry, something went wrong.

bollwyvl and others added 17 commits January 23, 2025 09:44
for more information, see https://pre-commit.ci

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…de/wheel install order
@bollwyvl bollwyvl closed this Jan 27, 2025
@bollwyvl bollwyvl reopened this Jan 27, 2025
@bollwyvl bollwyvl marked this pull request as ready for review January 27, 2025 22:40
@bollwyvl
Copy link
Contributor Author

@ryanking13 @hoodmane this should Do The Thing without breaking as much!

CHANGELOG.md Outdated
Comment on lines 11 to 13
- `micropip.install` now installs wheels from PyPI or custom indexes before built-in
Pyodide packages, reversing the previous behavior.
[#181](https://github.com/pyodide/micropip/pull/181)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a user-facing change related to this? I think the order of installation is an implementation detail that is not visible to the user. What a user sees whether the whole set of packages (with dependencies) are installed or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't know: seems like In interactive package management, which can be modified by about five different flags now, I think "reversing what we used to do," warrants at least a note. I am not sure if it's breaking, but things folk did before might not work now, and they might not need workarounds (like the double-install thing).

Copy link
Member

@ryanking13 ryanking13 Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Makes sense. Let me check the code more carefully and review.

I think the current wording could be a bit misleading to users though. When the same package exists in both the pyodide lock file and PyPI, the lockfile currently takes precedence (in terms of which package to choose, not the order of installation). This PR is not related to it, but maybe the user will think that this PR is changing that behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right: today, in the transaction phase once a dependency chain hits a pyodide package, further dependency resolution was skipped because the lockfile would take care of the rest. This was a performance win, for sure. In the install stage, they were all mixed together in wheel_tasks, and there was really no way to make any claims about what would happen.

With this change, the transaction will keep going all the way down for every novel dependency. In the install phase, the PyPI wheels get installed (in parallel) before calling loadPackage, which won't re-install (and re-download) a pyodide wheel.

This does mean there will be an extra add_dependency_inner for each depends item, but I couldn't figure out a way to do it that would be remotely correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... but that's a lot to say in a CHANGELOG, maybe there's a terser way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tried some more things on 92baa16

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, the transaction will keep going all the way down for every novel dependency. In the install phase, the PyPI wheels get installed (in parallel) before calling loadPackage, which won't re-install (and re-download) a pyodide wheel.

Hmm, I still don't quite get what which won't re-install means. By default, during the dependency resolution step, micropip will check if the package is available in the lockfile, and select the one from the lockfile instead of the one from the PyPI.

So, in the installation step, we expect no overlap between transaction.pyodide_packages and transaction.wheels. Does this PR change this behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the download of two pytest wheels:

image

After this PR, since the wheel would already be installed, it would look more like this:

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I didn't consider putting multiple packages in install(...). Thanks.

@ryanking13 ryanking13 mentioned this pull request Jan 29, 2025
@agriyakhetarpal agriyakhetarpal added this to the 0.9.0 milestone Jan 29, 2025
@bollwyvl
Copy link
Contributor Author

Will work on merge conflicts.

@bollwyvl bollwyvl requested a review from ryanking13 January 29, 2025 14:36
…raints-wheels-first
@bollwyvl
Copy link
Contributor Author

Merging main, looks like something is broken, but can't reproduce locally:

    @pytest.mark.skip_refcount_check
    @run_in_pyodide(packages=["micropip"])
    async def test_load_binary_wheel2(selenium_standalone_micropip):
        from pyodide_js._api import repodata_packages
    
        import micropip
    
>       await micropip.install(repodata_packages.regex.file_name)

tests/test_install.py:329: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/lib/python3.12/site-packages/micropip/package_manager.py:146: in install
    ???
/lib/python3.12/site-packages/micropip/install.py:52: in install
    ???
/lib/python3.12/site-packages/micropip/transaction.py:71: in gather_requirements
    ???
/lib/python3.12/site-packages/micropip/transaction.py:84: in add_requirement
    ???
/lib/python3.12/site-packages/micropip/transaction.py:180: in add_requirement_inner
    ???
/lib/python3.12/site-packages/micropip/transaction.py:227: in _add_requirement_from_package_index
    ???
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

>   ???
E   ValueError: Can't fetch metadata for 'regex-2024-9-11-cp312-cp312-pyodide-2024-0-wasm32-whl'. Please make sure you have entered a correct package name and correctly specified index_urls (if you changed them).

@bollwyvl
Copy link
Contributor Author

Ah, it looks like the .whl is being canonicalized as a package name into -whl

-wasm32-whl

... but also no idea why it's not failing locally...

@bollwyvl
Copy link
Contributor Author

Back to ✔️

Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me overall. Thanks for your work!

Comment on lines +59 to +61
if self.verbose and messages:
for constraint, msg in messages.items():
logger.info("Transaction: constraint %s discarded: %s", constraint, msg)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the long run, it seems better to change it so that the installation fails when constraints resolution fails (just like pip's behavior). However, micropip's current dependency resolution is incomplete on its own, so let's leave it for now.

@ryanking13 ryanking13 merged commit c9221a0 into pyodide:main Feb 1, 2025
8 checks passed
@bollwyvl bollwyvl deleted the gh-175-constraints-wheels-first branch February 1, 2025 15:52
@bollwyvl
Copy link
Contributor Author

bollwyvl commented Feb 1, 2025

Thanks all!

@agriyakhetarpal
Copy link
Member

Released in https://github.com/pyodide/micropip/releases/tag/v0.9.0, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support constraints for transient dependencies?
3 participants