-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
Modularized client for state management #114
Changes from 6 commits
9ccf2f8
9cf7f79
1a9586e
6b86cd5
838a706
d622cc1
0181b3f
ece5a92
31f4453
3d2bdd8
a1526ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,69 @@ | ||||||
from typing import Any, List | ||||||
|
||||||
from micropip import package_index | ||||||
|
||||||
|
||||||
class Manager: | ||||||
""" | ||||||
Manager provides an extensible interface for customizing micropip's behavior. | ||||||
|
||||||
Each Manager instance holds its own local state that is | ||||||
independent of other instances, including the global state. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Well it's not really global then. It's a bit unclear how this works since the result of installing stuff with micropip is generally files placed in the file system. I guess different There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant to phrase it in a way that the global manager instance(representing the 'global state' for the exposed commands) is also included in the statement, but maybe it isn't necessary to specify it as such. I would guess that the 'right way' to keep things independent would be to also have different install directories, but I'm not sure how nicely that would play out with sitepackages, or perhaps a mechanism to prevent managers having duplicate install directories. Could you provide your opinion on this? cc: @ryanking13 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ideally, we may try to have each instance behave completely independently as if each instance becomes a virtual environment. For example, by combining micropip and subinterpreter, we may be able to support different interpreters to install and run different sets of packages. However, I think most of the time, people will end up using one instance, and we would recommend doing so, too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is super useful for testing purposes but probably not as useful for real world stuff. How does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know That said, I know recent versions of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
My main goal of making micropip modular is to allow us (or others) to customize some of its behavior. It's nice to have each instance behave independently, but it's not really something I'm very interested in right now (of course we can benefit from it in testing purpose). |
||||||
|
||||||
TODO: Implement all of the following global commands to utilize local state. | ||||||
""" | ||||||
|
||||||
def __init__(self): | ||||||
self.index_urls = package_index.DEFAULT_INDEX_URLS | ||||||
|
||||||
# TODO: initialize the compatibility layer | ||||||
self.repodata_packages: dict[str, dict[str, Any]] = {} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
(no action required in this PR) |
||||||
self.repodata_info: dict[str, str] = {} | ||||||
|
||||||
pass | ||||||
|
||||||
def install(self): | ||||||
raise NotImplementedError() | ||||||
|
||||||
def list(self): | ||||||
raise NotImplementedError() | ||||||
|
||||||
def freeze(self): | ||||||
raise NotImplementedError() | ||||||
|
||||||
def add_mock_package(self): | ||||||
raise NotImplementedError() | ||||||
|
||||||
def list_mock_packages(self): | ||||||
raise NotImplementedError() | ||||||
|
||||||
def remove_mock_package(self): | ||||||
raise NotImplementedError() | ||||||
|
||||||
def uninstall(self): | ||||||
raise NotImplementedError() | ||||||
|
||||||
def set_index_urls(self, urls: List[str] | str): | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
from Python 3.9, standard types can be used as type helpers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was because of the name conflict with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, that's funny. It was probably a bad decision to call it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a comment next to the |
||||||
""" | ||||||
Set the index URLs to use when looking up packages. | ||||||
|
||||||
- The index URL should support the | ||||||
`JSON API <https://warehouse.pypa.io/api-reference/json/>`__ . | ||||||
|
||||||
- The index URL may contain the placeholder {package_name} which will be | ||||||
replaced with the package name when looking up a package. If it does not | ||||||
contain the placeholder, the package name will be appended to the URL. | ||||||
|
||||||
- If a list of URLs is provided, micropip will try each URL in order until | ||||||
it finds a package. If no package is found, an error will be raised. | ||||||
|
||||||
Parameters | ||||||
---------- | ||||||
urls | ||||||
A list of URLs or a single URL to use as the package index. | ||||||
""" | ||||||
|
||||||
if isinstance(urls, str): | ||||||
urls = [urls] | ||||||
|
||||||
self.index_urls = urls[:] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import micropip.package_index as package_index | ||
from micropip.manager import Manager | ||
|
||
|
||
def test_manager() -> Manager: | ||
manager = Manager() | ||
assert manager.index_urls == package_index.DEFAULT_INDEX_URLS | ||
|
||
return manager | ||
|
||
|
||
def test_set_index_urls(): | ||
manager = test_manager() | ||
|
||
default_index_urls = package_index.DEFAULT_INDEX_URLS | ||
assert manager.index_urls == default_index_urls | ||
|
||
valid_url1 = "https://pkg-index.com/{package_name}/json/" | ||
valid_url2 = "https://another-pkg-index.com/{package_name}" | ||
valid_url3 = "https://another-pkg-index.com/simple/" | ||
try: | ||
manager.set_index_urls(valid_url1) | ||
assert manager.index_urls == [valid_url1] | ||
|
||
manager.set_index_urls([valid_url1, valid_url2, valid_url3]) | ||
assert manager.index_urls == [valid_url1, valid_url2, valid_url3] | ||
finally: | ||
manager.set_index_urls(default_index_urls) | ||
assert manager.index_urls == default_index_urls |
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 like the name
Manager
? It's a bit vague but seems alright to me. We should consider whether we can come up with something better. @jaraco maybe you have some thoughts?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 don't love "Manager", but it's adequate.
I notice below the
repodata_
prefix on a couple of member variables, suggesting that maybe this object is representingRepoData
orState
orRepositoryState
orRepository
orRepo
(and maybe therepodata_
prefix can be removed). I also see that "repodata" is a legacy artifact and "lock" is relevant, so maybeLockState
or some variant.Since I see actions about
.install
, and.freeze
, I'm thinking this is relevant to anEnvironment
orSite
.I'm liking
Repository
orEnvironment
best, but I'd be okay with any option. Feel free to pick/promote one you like best.Am I right in thinking this class is an internal implementation detail (not part of a public API)? If so, the naming isn't critical and can be changed later.
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.
My thoughts were that eventually this class will become representative of the current public API, with additional extensibility such as dependency injection as discussed in #112, although I would guess having a separate exposed layer and keeping the
Manager
internal is also a feasible choice. (Assuming that the most common case of a user wanting to instantiate their ownManager
without accessing the current public APIs would be to utilize such extensibility)I was thinking that
Client
orMicropip
could suit it better butManager
seemed closest to what it was going to do: state management. I do agree its a vague name, andRepository
also sounds alright too.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.
Yep, I plan to make this class public. People would do
mgr = micropip.Manager(); mgr.install(...)
.I prefer
Manager
orEnvironment
. The former emphasizes that this is a package manager of sorts, while the latter recognizes that each instance is like a virtual environment of sorts.Let's go with
Manager
for now. We still have time before implementing all these APIs and making this class public.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.
Maybe call it
PackageManager
then? or???Environment
? I feel like we could use an extra word in the 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.
PackageManager
sounds good to me.