JohannesBuchner / imagehash

A Python Perceptual Image Hashing Module
BSD 2-Clause "Simplified" License
3.28k stars 331 forks source link

Make `PyWavelets` and `scipy` optional installs #195

Closed Avasam closed 11 months ago

Avasam commented 1 year ago

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.
coveralls commented 1 year ago

Coverage Status

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

JohannesBuchner commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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: https://github.com/pypa/pip/pull/10837 (issue: https://github.com/pypa/pip/issues/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)

Avasam commented 11 months ago

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

https://github.com/PyWavelets/pywt/issues/695 claims this is now resolved?

Avasam commented 11 months ago

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

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.