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

ENH Add micropip.uninstall() #55

Merged
merged 28 commits into from
Mar 15, 2023
Merged

ENH Add micropip.uninstall() #55

merged 28 commits into from
Mar 15, 2023

Conversation

ryanking13
Copy link
Member

@ryanking13 ryanking13 commented Mar 10, 2023

Related: #51

Add micropip.uninstall() method, which uninstalls packages that are installed.

I used a quite simple approach for this compared to pip uninstall, since we only support installing wheels, and wheel metadata contain a list of files in the archive, so I just iterated through the list and removed them.

I didn't add a rollback feature if uninstall fails in this PR. I'm not sure it is worth it, but probably we can add it as a follow up.

  • changelog

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks @ryanking13 ! A couple of minor comments otherwise LGTM.

What would happen if we uninstall a package installed via loadPackage? If it also uninstalls it, we need to be sure loadPackage would be aware of it.

micropip/uninstall.py Outdated Show resolved Hide resolved
micropip/uninstall.py Show resolved Hide resolved
micropip/uninstall.py Outdated Show resolved Hide resolved
@ryanking13
Copy link
Member Author

What would happen if we uninstall a package installed via loadPackage? If it also uninstalls it, we need to be sure loadPackage would be aware of it.

I think removing keys in loadedPackages does the job. Will add a test.

@ryanking13
Copy link
Member Author

@rth Thanks for the review! I think I addressed all of your comments.

I changed the code a bit after your review (simplified how files are retrieved, and added tests), so I would appreciate it if you can review this one more time.

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

One minor comment, otherwise LGTM. Thanks!

micropip/utils.py Outdated Show resolved Hide resolved
@ryanking13 ryanking13 merged commit a8b1d07 into pyodide:main Mar 15, 2023
@ryanking13 ryanking13 deleted the uninstall branch March 15, 2023 01:02
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.

2 participants