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

Minimize re-exports from __init__ files #44

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

akx
Copy link
Contributor

@akx akx commented Jul 17, 2023

Quoting the README:

General Philosophy

Modularity is king.

This allows importing parts of the package without having to import practically everything (since importing a package will import its parents' __init__s, etc).

The one alias I didn't touch is sgm.modules.GeneralConditioner since it is currently being referred to using that name in the YAML config files. I suppose the configs could be changed to do sgm.models.encoders.modules.GeneralConditioner too, but that didn't seem in scope for this.

@akx akx mentioned this pull request Jul 18, 2023
This allows importing parts of the package without having to
import practically everything (since importing a package will
import its parents' __init__s, etc).
@benjaminaubin
Copy link
Contributor

Why was it an issue to import sgm as a whole ?
If not issue we will keep it as it is

@akx
Copy link
Contributor Author

akx commented Jul 25, 2023

@benjaminaubin It is very much a problem.

This ties into the dependency discussion we had.

sgm.data imports a bunch of modules that are only required for training, not inference.

If you try to import, say, sgm.helpers.inference after #56 lands, it means sgm.helpers is imported, which means sgm is imported, and it imports sgm.data, and so on, and you'll end up needing a bunch of dependencies you shouldn't – not to mention it has an impact on import time too.

In short, you can't even import DiffusionEngine on main without having e.g. torchdata, webdataset, and then it'll complain about stable-datasets, which is supposed to be a submodule?

(main) $ python -c 'from sgm.models.diffusion import DiffusionEngine' 2>&1
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "generative-models/sgm/__init__.py", line 1, in <module>
    from .data import StableDataModuleFromConfig
  File "generative-models/sgm/data/__init__.py", line 1, in <module>
    from .dataset import StableDataModuleFromConfig
  File "generative-models/sgm/data/dataset.py", line 3, in <module>
    import torchdata.datapipes.iter
ModuleNotFoundError: No module named 'torchdata'
(main) $ pip install torchdata
(Installed torchdata and about 20 other deps)
(main) $ python -c 'from sgm.models.diffusion import DiffusionEngine' 2>&1
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "generative-models/sgm/__init__.py", line 1, in <module>
    from .data import StableDataModuleFromConfig
  File "generative-models/sgm/data/__init__.py", line 1, in <module>
    from .dataset import StableDataModuleFromConfig
  File "generative-models/sgm/data/dataset.py", line 4, in <module>
    import webdataset as wds
ModuleNotFoundError: No module named 'webdataset'
(main) $ pip install webdataset
(Installed webdataset and another dependency)
(main) $ python -c 'from sgm.models.diffusion import DiffusionEngine' 2>&1
######################################################################
Datasets not yet available
to enable, we need to add stable-datasets as a submodule
please use ``git submodule update --init --recursive``
and do ``pip install -e stable-datasets/`` from the root of this repo
######################################################################

Whereas on this PR's branch:

(minimize-re-exports) $ python -c 'from sgm.models.diffusion import DiffusionEngine' 2>&1
no module 'xformers'. Processing without...
no module 'xformers'. Processing without...
(minimize-re-exports) $

@akx akx mentioned this pull request Jul 25, 2023
@benjaminaubin benjaminaubin merged commit 57862fb into Stability-AI:main Jul 25, 2023
@akx
Copy link
Contributor Author

akx commented Jul 25, 2023

Thanks ❤️

@akx akx deleted the minimize-re-exports branch July 25, 2023 14:24
timudk added a commit that referenced this pull request Jul 25, 2023
jenuk pushed a commit that referenced this pull request Jul 26, 2023
LinearFalcon pushed a commit to LinearFalcon/generative-models that referenced this pull request Jul 6, 2024
This allows importing parts of the package without having to
import practically everything (since importing a package will
import its parents' __init__s, etc).
LinearFalcon pushed a commit to LinearFalcon/generative-models that referenced this pull request Jul 6, 2024
SevanBrodjian pushed a commit to SevanBrodjian/sd-latent-exploration that referenced this pull request Aug 12, 2024
This allows importing parts of the package without having to
import practically everything (since importing a package will
import its parents' __init__s, etc).
SevanBrodjian pushed a commit to SevanBrodjian/sd-latent-exploration that referenced this pull request Aug 12, 2024
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.

2 participants