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

Add torch to setup_requires & dynamic import to prevent import errors when installing via pip #514

Merged
merged 9 commits into from
Aug 30, 2022

Conversation

orgoro
Copy link
Contributor

@orgoro orgoro commented Mar 6, 2022

The problem

When trying to install BasicSR as part of of requirements.txt file, install fails if torch is not already installed
It is required in the setup phase
This behaviour can cause unexpected errors in depending on the pip install order

The Solution

  • In this PR I added setup_requires parameter to indicate that torch is required for the setup phase

  • Because torch is required only when building the cuda extensions I moved the import to be dynamic with a clear error message for the user

References

@orgoro
Copy link
Contributor Author

orgoro commented Apr 27, 2022

please approve @xinntao

@JCBrouwer
Copy link
Contributor

JCBrouwer commented Jul 19, 2022

This doesn't work for me. When torch is not installed, even when not building the extensions, the line cmdclass={'build_ext': BuildExtension}, at the end of setup() throws a NameError: name 'BuildExtension' is not defined.

A slightly edited working version here: https://github.com/JCBrouwer/BasicSR/blob/feature/dynamic-import-torch/setup.py

@abravalheri
Copy link

You can use pyproject.toml to make torch available before setup.py is processed.

Something like:

# pyproject.toml
[build-system]
requires = ["cython", "numpy", "torch", "setuptools"]
build-backend = "setuptools.build_meta"

build-backend = "setuptools.build_meta" will remove the CWD from sys.path inside your setup.py file.
If you need CWD in sys.path you can also use build-backend = "setuptools.build_meta:__legacy__".

@xinntao xinntao merged commit 3974c3f into XPixelGroup:master Aug 30, 2022
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.

None yet

4 participants