lyst / lightfm

A Python implementation of LightFM, a hybrid recommendation algorithm.
Apache License 2.0
4.66k stars 679 forks source link

Fix build for pip with --use-pep517 #661

Closed daniel-ferguson closed 11 months ago

daniel-ferguson commented 1 year ago

Resolves #660

When building this package using pip install --use-pep517 --no-binary :all: or in dev pip install -e . --use-pep517 we see a failure in setup.py:

  [...]
  AttributeError: 'dict' object has no attribute '__LIGHTFM_SETUP__'

__builtins__ is an implementation detail and:

The value of __builtins__ is normally either this module or the value of this module’s __dict__ attribute

When using pip with --use-pep517 it seems to be a dict, and otherwise seems to be the builtins object.

Rather than switch to importing the builtins module I've taken the opportunity to rework the package version code to follow approach (3) detailed in https://packaging.python.org/en/latest/guides/single-sourcing-package-version/

This feels a little more forgiving than the current approach in that it avoids loading the actual package itself in setup.py, skipping the workaround for the fact LightFM relies on a native extension.


Questions

TNonet commented 1 year ago

@maciejkula @SimonCW What is the best way to get this merged?

TNonet commented 1 year ago

@daniel-ferguson This PR seems to be failing the CI checks. Do you need assistance with this?

@maciejkula @SimonCW Is there a process or active maintainer to assist with merging this PR?

daniel-ferguson commented 1 year ago

@TNonet I've fixed the previous lint error but besides that I think there's a bit of bitrot at play, unrelated to changes in this PR, that should probably be resolved separately.

In the circle-ci lint step:

Traceback (most recent call last):
  File "/home/circleci/project/venv/bin/black", line 8, in <module>
    sys.exit(patched_main())
  File "src/black/__init__.py", line 1423, in patched_main
  File "src/black/__init__.py", line 1409, in patch_click
ImportError: cannot import name '_unicodefun' from 'click' (/home/circleci/project/venv/lib/python3.8/site-packages/click/__init__.py)

In the appveyor build it looks like there are native code compilation errors, e.g.

...
lightfm/_lightfm_fast_no_openmp.c(24538): error C2105: '++' needs l-value
lightfm/_lightfm_fast_no_openmp.c(24540): error C2105: '--' needs l-value
lightfm/_lightfm_fast_no_openmp.c(24849): error C2105: '++' needs l-value
lightfm/_lightfm_fast_no_openmp.c(24851): error C2105: '--' needs l-value
lightfm/_lightfm_fast_no_openmp.c(25099): error C2105: '++' needs l-value
lightfm/_lightfm_fast_no_openmp.c(25101): error C2105: '--' needs l-value
...

Resolving these would definitely need more of a time investment than I'm capable of at the moment.

daniel-ferguson commented 11 months ago

Closing as this was resolved in an updated version of this PR: https://github.com/lyst/lightfm/pull/685