-
-
Notifications
You must be signed in to change notification settings - Fork 28
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 add_mock_package #26
Conversation
for more information, see https://pre-commit.ci
…ropip into add_package_mock
for more information, see https://pre-commit.ci
path_parts = n.split(".") | ||
dir_path = Path(site_packages, *path_parts) | ||
os.makedirs(dir_path, exist_ok=True) | ||
init_file = dir_path / "__init__.py" |
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 actually need to create these physical files?
I was thinking we could maybe directly initialize sys.modules
with the mapping from modules: dict[str, ModuleType | str]
? That way it's also easy to use MagicMock
as a module or any other custom object defined at runtime to make a given dependency work.
Though yes, the API to create modules dynamically via importlib.util.module_from_spec
or by inheriting from ModuleType isn't very straightforward.
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 making another custom Finder/Loader would be a good way to go. The API for that isn't too bad.
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 actually wrote it initially using mock objects and putting them into sys.modules, but I reverted to writing them out to files. I did it like that because it is really simple and reliable, things like file paths etc. just work. It also works nicely in terms of loading modules, in that init code is only run on actual import. Oh and in the case that you're running somewhere with persistent file system, your mock modules continue to exist on reload of python.
It's a pretty minor change - you just create a module and exec the code into it in the loader, I can put that back in if you reckon the possibility of using mock objects makes sense.
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.
Actually I think rather than take an object it needs to take a callable which takes a module object and does whatever needs to be done on it to make it. I.e. the equivalent of loader.exec_module
That way you can mock module initialisation correctly.
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.
One further thought on this - at some point I want to implement some kind of mega wheel option, to bundle a set of wheels and all deps into a single wheel with no deps. For that use case keeping persistent mock modules would be nice. If it's okay I'd like to keep that as an option in micropip?
It would also be nice for any point that pyodide is run in a persistent file system.
I'm thinking that a flag "persistent" could be added (which only works for string modules) and then the behaviour with finder and loader could be used in the case that the flag isn't set.
I think this is reasonable, as having a physical file would be a most easy way to keep the state of installed packages or to bundle packages (for example, in We can change the implementation of |
micropip/_micropip.py
Outdated
s = dedent(s) | ||
path_parts = n.split(".") | ||
dir_path = Path(site_packages, *path_parts) | ||
os.makedirs(dir_path, exist_ok=True) |
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 we should raise error if a directory exists, because it would mean that the package is already installed somehow.
micropip/_micropip.py
Outdated
@@ -692,3 +695,72 @@ def _list(): | |||
source_ = pkg_source | |||
packages[name] = PackageMetadata(name=name, version=version, source=source_) | |||
return packages | |||
|
|||
|
|||
def add_mock_package(name, version, *, modules=None): |
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.
Could you please add type hints here?
os.makedirs(dir_path, exist_ok=True) | ||
init_file = dir_path / "__init__.py" | ||
with open(init_file, "w") as f: | ||
f.write(s) |
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 should keep the list of mocked modules, so it can be accessed via e.g. micropip.mock_modules
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.
Added list_mock_modules instead - which uses module installer metadata to list modules it has installed
for more information, see https://pre-commit.ci
…ropip into add_package_mock
for more information, see https://pre-commit.ci
…ropip into add_package_mock
for more information, see https://pre-commit.ci
…ropip into add_package_mock
@ryanking13 @rth I've added support for non-persistent mocks, and made the tests work in pyodide. I think I've fixed the things from the reviews above. There's a bit more code in there to do the in-memory mocking, which is probably worth a look over before merging. |
…ropip into add_package_mock
Oh and it supports listing and removing mocks also btw. |
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. Having a persistent parameter seems good to me. Overall LGTM, if @rth or @hoodmane is also +1 with this, I'll merge.
Could you please also update the changelog? Probably we can follow the format of KeepAChangelog
micropip/_micropip.py
Outdated
with open(init_file, "w") as f: | ||
f.write(s) | ||
with open(installer_file, "w") as f: | ||
f.write("micropip mock package") |
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 move these hard coded INSTALLER types into a variables?
micropip/_mock_package.py
Outdated
|
||
|
||
_finder = _MockModuleFinder() | ||
sys.meta_path = [_finder] + sys.meta_path |
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'm not sure we want an importing micropip to have a side effect of modifying sys.meta_path
, though it does not have any effect when no mock packages are registered.
Perhaps modify sys.meta_path when add_in_memory_distribution
is first called?
tests/test_micropip.py
Outdated
|
||
from micropip._micropip import add_mock_package | ||
|
||
with TemporaryDirectory() as tmpdirname: |
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.
There is a tmp_path
fixture for this purpose.
tests/test_micropip.py
Outdated
# check package removes okay | ||
_micropip.remove_mock_package("micropip_test_bob") | ||
del micropip_bob_mod | ||
try: |
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 use with pytest.raises(...)
instead?
tests/test_micropip.py
Outdated
# check package removes okay | ||
_micropip.remove_mock_package("micropip_test_bob") | ||
del micropip_bob_mod | ||
try: |
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 here (pytest.raises
)
for more information, see https://pre-commit.ci
Okay, done those bits and pieces now and changelog. Good spot about the sys.meta_path thing, that was silly. |
…ropip into add_package_mock
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 a lot for this work @joemarshall and @ryanking13 for the review!
} | ||
`` | ||
|
||
Parameters |
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 there is an indentation issue in this docstring, but we can fix it once we start actually building docs for it.
I guess we should release 0.1.1 (or 0.2.0) with this. Though probably we need to start building docs for micropip before making a release. |
Added a method to micropip to allow easy mocking of packages (for use in jupyterlite / pyodide)
It takes package name and version, along with a list of implemented modules and optionally their code if needed.