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

I like my coffee black, my py4DSTEM lite, and my tea in the river #668

Merged
merged 11 commits into from
Jul 25, 2024
Merged

Conversation

gvarnavi
Copy link
Member

@gvarnavi gvarnavi commented Jul 4, 2024

Various import changes to make py4DSTEM import without "heavy" dependencies.
This allows the new py4DSTEM-lite repo to simply include this repo as a submodule with only minor changes to its setup.py. You can see a live example here: https://marimo.app/?slug=xct4m5.

Note this should ideally not affect any default behaviour, so if people could checkout this branch and check their workflows, that would be appreciated.

The only functionality change I had to make is disabling the configuration_checker since it uses package-specific metadata (and thus the submodule can't use it). I'll look into how to support this before this is merged.

@gvarnavi gvarnavi requested review from sezelt, smribet and bsavitzky July 4, 2024 17:06
@sezelt
Copy link
Member

sezelt commented Jul 7, 2024

I'm concerned that having so many import exceptions being caught and discarded will interfere with our CI checks, since any bugs we introduce in these areas will no longer be able to throw import errors. It would be good if we knew at import time whether we were running in "lite" mode, so that we can decide to raise or ignore these errors.

@gvarnavi
Copy link
Member Author

gvarnavi commented Jul 8, 2024

The idea here was to make the code base run w/ or w/o the "heavy" packages so it can be used as a git submodule, but yeah I hear you..

Perhaps the way to do it is to specify a "lite" metadata entry on each of the setup.py files and read that on import / catch import errors accordingly?

@sezelt
Copy link
Member

sezelt commented Jul 8, 2024

Can the distribution metadata handle arbitrary entries? From a quick glance it looks like you are limited to the standard information that is used for packaging. I suppose if we are going to make a separate distribution then it could check the installed name. I wonder if our long-standing config file idea would be useful here. The lite version could be a distribution that contains just a premade config file that has lite=True and depends on py4DSTEM, and these heavy imports could ignore errors iff in lite mode. (I don't know how the mechanics of making a distribution like this would work btw, but it feels possible).

@gvarnavi
Copy link
Member Author

gvarnavi commented Jul 9, 2024

Not sure about arbitrary metadata, but it at the very least supports "keywords" -- so we could check if that list includes the keyword "lite".

A config file would definitely be a cleaner way to handle this -- but I don't want this to get bogged down in a more general discussion on config files.

I'll prototype something based on the metadata idea and we can discuss if that suits our needs at the dev meeting on Thursday?

@gvarnavi
Copy link
Member Author

Ok, this ended up being slightly more complicated than I imagined, since both distributions have the same top-level import name (py4DSTEM) by design, but the following line in py4DSTEM/__init__.py seems to work fine:

from importlib.metadata import packages_distributions

is_package_lite = "py4DSTEM-lite" in packages_distributions()["py4DSTEM"]

Essentially, we're checking if there's a py4DSTEM-lite distribution installed in the environment with the top-level import name py4DSTEM, and only raise ImportError/ModuleNotFoundError errors accordingly if the distribution is not lite.

This should also be easy enough to update if we ever do switch to a config file.

@sezelt
Copy link
Member

sezelt commented Jul 11, 2024

This looks like it will work, though the downside is that if you have py4DSTEM lite installed in an environment it seems that it is not possible to import the full package anymore. Probably for the use cases you're envisioning this is not a problem but I can see this being a problem if people aren't aware of the distinction.

@gvarnavi
Copy link
Member Author

@sezelt Indeed, this was rather annoying since all the built-in magic variables (such as __package__ and __spec__) point to the top-level import py4DSTEM and not the distribution name). As far as I could tell, there's no robust way to know the distribution name programmatically.

To clarify, it's not that you can't install regular py4DSTEM if you have a lite installation, since the two are functionally identical, just that it won't perform the import checks.

Perhaps it's worth adding a line in the README file of the sort:

_Note:_ If this is not a fresh environment, and you've installed the `py4DSTEM-lite` package before,  
it might be worth uninstalling that first, before installing the core `py4DSTEM` package:  
`pip uninstall py4DSTEM-lite && pip install py4DSTEM`

@bsavitzky bsavitzky merged commit ab61525 into dev Jul 25, 2024
8 checks passed
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