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

Port freeze into PackageManager #115

Merged
merged 5 commits into from
Jun 28, 2024
Merged

Conversation

RulerOfCakes
Copy link
Contributor

After #114, I'm continuing to implement each command into PackageManager.

As a relatively simple first change freeze() now uses the given state instead of the global variables.

Note that the current implementation of freeze still reads from importlib.metadata.distributions(), so it isn't completely independent from the global environment. This isn't as trivially separable as the repodata_* variables due to certain intrinsic quirks as discussed in the previous PR, so for now I'm leaving the path/site-packages/install related elements as-is.

@RulerOfCakes
Copy link
Contributor Author

RulerOfCakes commented Jun 24, 2024

After the discussion about potentially having separate install directories in the previous PR, I've taken a look at tox and from a quick overview it looks like their approach is to create an entirely separate python environment with virtualenv and avoid juggling with path / site-packages in the same environment. I'm unsure how to potentially incorporate this approach into micropip to work with pyodide as it doesn't seem to be an easy task, imo it feels like it would be better to leave it as a TODO for now. Any thoughts? cc: @ryanking13

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.

Thanks @RulerOfCakes!

micropip/_commands/freeze.py Outdated Show resolved Hide resolved
micropip/_commands/freeze.py Outdated Show resolved Hide resolved
micropip/package_manager.py Outdated Show resolved Hide resolved
tests/test_package_manager.py Outdated Show resolved Hide resolved
@ryanking13
Copy link
Member

ryanking13 commented Jun 25, 2024

Note that the current implementation of freeze still reads from importlib.metadata.distributions(), so it isn't completely independent from the global environment. This isn't as trivially separable as the repodata_* variables due to certain intrinsic quirks as discussed in the #114 (comment), so for now I'm leaving the path/site-packages/install related elements as-is.

imo it feels like it would be better to leave it as a TODO for now. Any thoughts?

Yes, this can be quite complicated, and I am not even sure it makes sense for Pyodide. so I think we can work on it separately later, for now you don't need to worry about making the PackageManager work independently.



def _freeze(
repodata_packages: dict[str, dict[str, Any]], repodata_info: dict[str, str]
Copy link
Member

Choose a reason for hiding this comment

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

(No action required for this PR)

For better typing and test coverage, we can think of using pyodide-lock package (which generates these repodata lock file) #88.

from typing import TYPE_CHECKING

if TYPE_CHECKING:
  from pyodide_lock import PackageSpec, InfoSpec
else:
  PackageSpec = ...
  InfoSpec = ...

_freeze(packages: PackageSpec, info: InfoSpec)

@RulerOfCakes RulerOfCakes marked this pull request as ready for review June 25, 2024 14:24
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.

Thanks @RulerOfCakes!

def test_freeze():
manager = get_test_package_manager()

test_repodata_info = {
Copy link
Member

Choose a reason for hiding this comment

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

Later, I think we can improve this by using an actual lock file structure.

@ryanking13 ryanking13 merged commit 8b39458 into pyodide:main Jun 28, 2024
5 checks passed
@hoodmane
Copy link
Member

Thanks @RulerOfCakes!

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.

3 participants