py4dstem / py4DSTEM

GNU General Public License v3.0
202 stars 140 forks source link

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

Closed gvarnavi closed 2 months ago

gvarnavi commented 3 months ago

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.

sezelt commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@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`