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

Make PyWavelets and scipy optional installs #195

Closed

Conversation

Avasam
Copy link

@Avasam Avasam commented Oct 20, 2023

It's generally nice to not install dependencies the user doesn't need on their machine.
But this also makes imagehash installable on Python 3.12 ! (see error below)

I also improved the error message if the dependency is missing.

PS C:\Users\Avasam> python3.12 -m pip install imagehash
Collecting imagehash
  Using cached ImageHash-4.3.1-py2.py3-none-any.whl (296 kB)
Collecting PyWavelets (from imagehash)
  Downloading PyWavelets-1.4.1.tar.gz (4.6 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 4.6/4.6 MB 12.2 MB/s eta 0:00:00
  Installing build dependencies ... done
  Getting requirements to build wheel ... error
  error: subprocess-exited-with-error

  × Getting requirements to build wheel did not run successfully.
  │ exit code: 1
  ╰─> [33 lines of output]
      Traceback (most recent call last):
        File "C:\Users\Avasam\AppData\Local\Programs\Python\Python312\Lib\site-packages\pip\_vendor\pyproject_hooks\_in_process\_in_process.py", line 353, in <module>
          main()
        File "C:\Users\Avasam\AppData\Local\Programs\Python\Python312\Lib\site-packages\pip\_vendor\pyproject_hooks\_in_process\_in_process.py", line 335, in main
          json_out['return_val'] = hook(**hook_input['kwargs'])
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        File "C:\Users\Avasam\AppData\Local\Programs\Python\Python312\Lib\site-packages\pip\_vendor\pyproject_hooks\_in_process\_in_process.py", line 112, in get_requires_for_build_wheel
          backend = _build_backend()
                    ^^^^^^^^^^^^^^^^
        File "C:\Users\Avasam\AppData\Local\Programs\Python\Python312\Lib\site-packages\pip\_vendor\pyproject_hooks\_in_process\_in_process.py", line 77, in _build_backend
          obj = import_module(mod_path)
                ^^^^^^^^^^^^^^^^^^^^^^^
        File "C:\Users\Avasam\AppData\Local\Programs\Python\Python312\Lib\importlib\__init__.py", line 90, in import_module
          return _bootstrap._gcd_import(name[level:], package, level)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        File "<frozen importlib._bootstrap>", line 1381, in _gcd_import
        File "<frozen importlib._bootstrap>", line 1354, in _find_and_load
        File "<frozen importlib._bootstrap>", line 1304, in _find_and_load_unlocked
        File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
        File "<frozen importlib._bootstrap>", line 1381, in _gcd_import
        File "<frozen importlib._bootstrap>", line 1354, in _find_and_load
        File "<frozen importlib._bootstrap>", line 1325, in _find_and_load_unlocked
        File "<frozen importlib._bootstrap>", line 929, in _load_unlocked
        File "<frozen importlib._bootstrap_external>", line 994, in exec_module
        File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
        File "C:\Users\Avasam\AppData\Local\Temp\pip-build-env-q96r8w2t\overlay\Lib\site-packages\setuptools\__init__.py", line 16, in <module>
          import setuptools.version
        File "C:\Users\Avasam\AppData\Local\Temp\pip-build-env-q96r8w2t\overlay\Lib\site-packages\setuptools\version.py", line 1, in <module>
          import pkg_resources
        File "C:\Users\Avasam\AppData\Local\Temp\pip-build-env-q96r8w2t\overlay\Lib\site-packages\pkg_resources\__init__.py", line 2191, in <module>
          register_finder(pkgutil.ImpImporter, find_on_path)
                          ^^^^^^^^^^^^^^^^^^^
      AttributeError: module 'pkgutil' has no attribute 'ImpImporter'. Did you mean: 'zipimporter'?
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.
error: subprocess-exited-with-error

× Getting requirements to build wheel did not run successfully.
│ exit code: 1
╰─> See above for output.

note: This error originates from a subprocess, and is likely not a problem with pip.

@Avasam Avasam changed the title Make PyWavelets and scipy optional installs Make PyWavelets and scipy optional installs + Fix Python 3.12 installation Oct 20, 2023
setup.py Outdated
Comment on lines 31 to 34
extras_require={
"whash": "PyWavelets", # for whash
'phash': "scipy", # for phash
},
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't have to provide these. Users can just add to their own dependencies. But it's a nice to have. And helps some tools that look at packages co-dependencies (for security analysis for instance)

@coveralls
Copy link

coveralls commented Oct 20, 2023

Coverage Status

coverage: 85.185% (-2.7%) from 87.847%
when pulling 1fcd3eb on Avasam:PyWavelets-scipy-optional
into 0624854 on JohannesBuchner:master.

@JohannesBuchner
Copy link
Owner

I can somewhat see the benefit of providing an escape route if scipy or pywavelet is not installable. But it seems better to me to by default install dependencies so that all features can be used.

@Avasam
Copy link
Author

Avasam commented Oct 21, 2023

I could always fork imagehash until PyWavelets 1.5 releases. Figured I'd try offering a more long-term solution upstream first.
In the meantime I'm also blocked on PySide6 anyway.

@JohannesBuchner
Copy link
Owner

I like most of the PR.

Is there a way to make the default install behave as if your [whash,phash] options are set?
Or is the only way to defined the reverse, a without-whash and a without-phash extra option?

An alternative workaround is to install with pip --no-dependencies.

@Avasam
Copy link
Author

Avasam commented Oct 21, 2023

Is there a way to make the default install behave as if your [whash,phash] options are set? Or is the only way to defined the reverse, a without-whash and a without-phash extra option?

Not as far as I know, it'd be more like a imagehash[all].

An alternative workaround is to install with pip --no-dependencies.

That said, I didn't know about --no-dependencies. Looking further into it, there's even a PR open to make the flag usable in requirements files, however there's a good chunk of discussion indicating uncertainty as per the "proper" fix for my scenario that prompted this PR: pypa/pip#10837 (issue: pypa/pip#9948)

Since I already end up having to use a custom install script for different reasons, I could restrict imagehash < Python 3.12, and add a pip install scipy imagehash --no-deps line in my script for Python 3.12. (in theory this should all resolve itself eventually)

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@Avasam
Copy link
Author

Avasam commented Dec 13, 2023

Is this better? It keeps the existing default install behaviour, but more clearly offers, and explains how to, skip what are considered optional installs.

@JohannesBuchner
Copy link
Owner

PyWavelets/pywt#695 claims this is now resolved?

@Avasam Avasam changed the title Make PyWavelets and scipy optional installs + Fix Python 3.12 installation Make PyWavelets and scipy optional installs Dec 14, 2023
@Avasam
Copy link
Author

Avasam commented Dec 14, 2023

PyWavelets/pywt#695 claims this is now resolved?

My PyWavelets install issue was resolved by --no-dependencies.

You mentioned liking part of the PR so I reduced the changes to just the optional runtime import and better error messages if it's a supported use-case.

I can somewhat see the benefit of providing an escape route if scipy or pywavelet is not installable. But it seems better to me to by default install dependencies so that all features can be used.

If you don't see value in these changes. You are welcome to close the PR.

@JohannesBuchner
Copy link
Owner

Hmm, yes. If pip cannot install pywavelets and scipy, there is something seriously wrong or some specialized use case. Thank you for exploring this direction, maybe we can revisit it in the future.

@Avasam Avasam deleted the PyWavelets-scipy-optional branch December 15, 2023 14:27
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