Adopt pep517 + try compile with cython #1709

CaselIT · 2020-04-03T20:19:35Z

Summary of Changes

This is just a WIP POC to see if something like this could work.

Add support for pep517 using the setuptools backend. This is based on Document setuptools.build_meta pypa/setuptools#1698 since currently no documentation exists.
Since we now use pep517 we have cython available during a normal install, so setuy.py has been updated to try to compile using cython and then falling back to a normal install.
This logic is based mostly on SQLAlchemy setup.py

How do I test it

# to install with cython, will fallback to pure-python if no compiler are installed, or skip it when using pypy
pip install -v git+https://github.com/falconry/falcon@refs/pull/1709/head
# to install without cython
FALCON_DISABLE_CYTHON=y pip install -v git+https://github.com/falconry/falcon@refs/pull/1709/head

See: #1709 (comment)

Related Issues

#1683

Changes:

use pep517
automatically compile cython if possible, fallback to pure python if no compiler installed
allow disabling cython compilation via env variable
remove --no-use-pep517 from readme
update wheel generation script
re-add pyproject in source dist
test wheel generation publishing to test pypi

codecov · 2020-04-03T20:52:34Z

Codecov Report

Merging #1709 (31762a6) into master (66e19f6) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master     #1709   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           63        63           
  Lines         6610      6610           
  Branches      1067      1067           
=========================================
  Hits          6610      6610

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66e19f6...31762a6. Read the comment docs.

setup.py

CaselIT · 2020-04-30T16:22:16Z

@vytas7 @kgriffs I'm not sure how we could test how robust is this solution.

From experience in sqlalchemy, pep517 is still not very stable right now, and numerous install issues have surfaced once the pyproject.toml was included in the distribution.
Currently the best solution found in sqlalchemy is to omit it from the distribution using the manifest.in file.

vytas7 · 2020-08-04T19:13:40Z

I tend to agree it is probably worth explicitly removing pyproject.toml from sdist (as in SQLAlchemy's MANIFEST.in).

Note that people installing straight from GitHub (either directly with pip, or cloning first) would nevertheless be still affected.

setup.py

vytas7 · 2020-12-16T20:32:36Z

@CaselIT just checking if you are determined to continue this endeavour, or should we just create a new PR to remove pyproject.toml from the sdist?

CaselIT · 2020-12-16T21:29:06Z

Yes we could continue this endeavor, I mean it's all a lot less relevant now since we ship wheels.

but I guess sooner or later pip will start complaining that the install is not pep517 compliant

kgriffs · 2020-12-16T23:26:47Z

If we are going to circle back on this post-3.0 do we want to remove pyproject.toml for now?

CaselIT · 2020-12-16T23:35:50Z

I think that it may be a great idea for now, until we figure a stable implementation for pep517

On sqlalchemy side we have had no problems since we removed it from the pypi package via the manifest file

CaselIT · 2021-05-13T19:40:33Z

shall we resurrect this one?

vytas7 · 2021-05-14T13:12:10Z

I'm absolutely for it, but the question is whether there is an easy way to do this...

Is there a way to support editable (-e .) installs for development? It's a bit of a relic from the egg-times, but still probably the most convenient way to link the source tree to the current virtual environment. Maybe Pip just does the right thing?
Is there a way to disable cythonization?
If cythonization fails (e.g., no compiler available), is there an easy way to proceed and fall back to pure Python when building? (This one is probably not critical, but highly desirable.)

Some packages opt to ship .c files, but I don't think it helps that much, since Cython is always available from PyPi.

CaselIT · 2021-05-14T13:19:21Z

Is there a way to support editable (-e .) installs for development? It's a bit of a relic from the egg-times, but still probably the most convenient way to link the source tree to the current virtual environment. Maybe Pip just does the right thing?

this should work in any case. note that since we have a pyproject.toml file we are actually already using pep517 in legacy mode

Is there a way to disable cythonization?

we could add an env variable, like sqlalchemy. I don't think pip accepts arguments that can be passed down on install.

If cythonization fails (e.g., no compiler available), is there an easy way to proceed and fall back to pure Python when building? (This one is probably not critical, but highly desirable.)

sure, it's already what this does, if it fails it falls back to the pure python install.

Some packages opt to ship .c files, but I don't think it helps that much, since Cython is always available from PyPi.

using pep517 that's not needed, since we can list cython as a build time requirement so it's always available when installing

CaselIT · 2021-05-14T20:24:52Z

Updated

CaselIT · 2021-05-14T20:37:35Z

Could you try to install falcon using this branch to see if some issues came up in some cases?
Can install using. It will compile with cython and will fallback to pure-python if no compiler are installed, or skip it when using pypy

pip install -v git+https://github.com/falconry/falcon@refs/pull/1709/head

of without cython

FALCON_DISABLE_CYTHON=y pip install -v git+https://github.com/falconry/falcon@refs/pull/1709/head

vytas7 · 2021-05-15T06:29:32Z

setup.py

+        run_setup(False)
+
+        status_msgs(
+            'Cython compilation could not be completed, speedups are not enabled.',


Wouldn't this message be repeated twice in the case pure Python build succeeds?

Consider that this is only visible if the verbose flag is passed (aka almost no-one)
This is what is printed:

building 'falcon.api_helpers' extension *************************************************************************** Microsoft Visual C++ 14.0 or greater is required. Get it with "Microsoft C++ Build Tools": https://visualstudio.microsoft.com/visual-cpp-build-tools/ Cython compilation could not be completed, speedups are not enabled. Failure information, if any, is above. Retrying the build without the C extension now. *************************************************************************** running develop running egg_info writing falcon.egg-info\PKG-INFO writing dependency_links to falcon.egg-info\dependency_links.txt writing entry points to falcon.egg-info\entry_points.txt writing top-level names to falcon.egg-info\top_level.txt warning: the 'license_file' option is deprecated, use 'license_files' instead adding license file 'LICENSE' (matched pattern 'LICENSE') reading manifest file 'falcon.egg-info\SOURCES.txt' reading manifest template 'MANIFEST.in' writing manifest file 'falcon.egg-info\SOURCES.txt' running build_ext Creating c:\users\cfede\miniconda3\envs\falcon\lib\site-packages\falcon.egg-link (link to .) Adding falcon 3.0.2.dev1 to easy-install.pth file Installing falcon-bench-script.py script to C:\Users\cfede\miniconda3\envs\falcon\Scripts Installing falcon-bench.exe script to C:\Users\cfede\miniconda3\envs\falcon\Scripts Installing falcon-inspect-app-script.py script to C:\Users\cfede\miniconda3\envs\falcon\Scripts Installing falcon-inspect-app.exe script to C:\Users\cfede\miniconda3\envs\falcon\Scripts Installing falcon-print-routes-script.py script to C:\Users\cfede\miniconda3\envs\falcon\Scripts Installing falcon-print-routes.exe script to C:\Users\cfede\miniconda3\envs\falcon\Scripts Installed c:\users\cfede\dev\github\falcon *************************************************************************** Cython compilation could not be completed, speedups are not enabled. Pure-Python build succeeded. *************************************************************************** Successfully installed falcon

I think in makes sense, maybe we could invert the working:

Pure-Python build succeeded. Cython compilation could not be completed, speedups are not enabled.

CaselIT · 2021-05-15T08:35:05Z

.github/workflows/create-wheels.yaml

@@ -15,6 +15,7 @@ jobs:
  # four jobs are defined make-wheel-win-osx, make-wheel-linux, make-source-dist and make-emulated-wheels
  # the wheels jobs do the the same steps, but linux wheels need to be build to target manylinux
  make-wheel-win-osx:
+    needs: make-source-dist


I've added a needs, since in the source dist we verify that the version is correct

vytas7

This looks good, well done. But I'm yet to test various scenarios myself.

README.rst

CaselIT · 2021-05-20T20:44:07Z

I've tried to install in docker on

debian:10
alpine
centos:8 and 7 (pip requires updating since it's v9)

and I did not have issues, both with or without c compiler

vytas7

This works well on every platform I've tested: PyPy3, Pyston, various odd CPython 3.5 combinations, 3.10.0b1 in Docker.

LGTM 💯

README.rst

setup.cfg

kgriffs

Just a couple of nits and this is good to go IMO. Nice work, esp. considering how poor the pep517 docs are.

CaselIT · 2021-05-25T21:11:39Z

@kgriffs updated.

… clarity

CaselIT · 2021-05-26T07:34:24Z

Thanks for the documentation fixes @kgriffs

README.rst

kgriffs

I think this is ready to rock. ⚡

vytas7

🤘

vytas7 reviewed Apr 4, 2020

View reviewed changes

setup.py Outdated Show resolved Hide resolved

vytas7 reviewed Aug 6, 2020

View reviewed changes

setup.py Outdated Show resolved Hide resolved

vytas7 reviewed Aug 6, 2020

View reviewed changes

setup.py Outdated Show resolved Hide resolved

kgriffs added the pr:delay-for-next-release label Dec 18, 2020

kgriffs mentioned this pull request Dec 22, 2020

chore(build): exclude pyproject.toml to avoid inadvertently triggerin… #1831

Merged

kgriffs removed the pr::delay-for-next-release label May 10, 2021

CaselIT added 2 commits May 14, 2021 21:40

chore(setup): add pep517 support and cython autocompile if possible

17dc29f

chore(setup): extract most setup information to setup.cfg

b7d31a6

CaselIT force-pushed the pep517 branch from af9d285 to b7d31a6 Compare May 14, 2021 20:02

chore(setup): allow env variable to disable cythonization

06fc3cf

CaselIT marked this pull request as ready for review May 14, 2021 20:07

chore(tox): update tox commands

96ee8ba

CaselIT changed the title ~~[POC] Adopt pep517 + try compile with cython~~ Adopt pep517 + try compile with cython May 14, 2021

style: improve pyproject formatting

8d6ce0f

CaselIT requested review from kgriffs, jmvrbanac and nZac May 14, 2021 20:30

vytas7 reviewed May 15, 2021

View reviewed changes

chore: update create-wheel workflow

d75204c

CaselIT commented May 15, 2021

View reviewed changes

chore(build): re-add pyproject in source build

32c9b64

vytas7 mentioned this pull request May 18, 2021

Improve Pyston support #1918

Open

3 tasks

vytas7 reviewed May 18, 2021

View reviewed changes

README.rst Outdated Show resolved Hide resolved

docs(readme): reword some install parts as per review

e6508c4

CaselIT requested a review from vytas7 May 19, 2021 20:47

Merge branch 'master' into pep517

9840542

vytas7 previously approved these changes May 22, 2021

View reviewed changes

kgriffs reviewed May 24, 2021

View reviewed changes

README.rst Show resolved Hide resolved

kgriffs reviewed May 24, 2021

View reviewed changes

setup.cfg Show resolved Hide resolved

kgriffs requested changes May 24, 2021

View reviewed changes

chore: address review feedback

346df0f

CaselIT dismissed vytas7’s stale review via 346df0f May 25, 2021 21:11

CaselIT requested review from vytas7 and kgriffs May 25, 2021 21:11

kgriffs added 2 commits May 25, 2021 15:14

doc(README): minor grammar fix

9d4084d

doc(README): minor tweak to installation instructions for grammar and…

77eb362

… clarity

CaselIT mentioned this pull request May 25, 2021

style: adopt black #1876

Merged

4 tasks

vytas7 reviewed May 26, 2021

View reviewed changes

README.rst Outdated Show resolved Hide resolved

docs: clarify sentence

31762a6

CaselIT requested a review from vytas7 May 26, 2021 08:51

kgriffs approved these changes May 26, 2021

View reviewed changes

vytas7 approved these changes May 26, 2021

View reviewed changes

vytas7 merged commit c550eb3 into falconry:master May 26, 2021

CaselIT deleted the pep517 branch May 26, 2021 18:06

vytas7 mentioned this pull request May 26, 2021

Adopt PEP 517 #1683

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt pep517 + try compile with cython #1709

Adopt pep517 + try compile with cython #1709

CaselIT commented Apr 3, 2020 •

edited

Loading

codecov bot commented Apr 3, 2020 •

edited

Loading

CaselIT commented Apr 30, 2020

vytas7 commented Aug 4, 2020 •

edited

Loading

vytas7 commented Dec 16, 2020

CaselIT commented Dec 16, 2020 •

edited

Loading

kgriffs commented Dec 16, 2020

CaselIT commented Dec 16, 2020

CaselIT commented May 13, 2021

vytas7 commented May 14, 2021

CaselIT commented May 14, 2021

CaselIT commented May 14, 2021

CaselIT commented May 14, 2021 •

edited

Loading

vytas7 May 15, 2021

CaselIT May 15, 2021

CaselIT May 15, 2021 •

edited

Loading

vytas7 left a comment

CaselIT commented May 20, 2021 •

edited

Loading

vytas7 left a comment

kgriffs left a comment

CaselIT commented May 25, 2021

CaselIT commented May 26, 2021

kgriffs left a comment

vytas7 left a comment

Adopt pep517 + try compile with cython #1709

Adopt pep517 + try compile with cython #1709

Conversation

CaselIT commented Apr 3, 2020 • edited Loading

Summary of Changes

How do I test it

Related Issues

Changes:

codecov bot commented Apr 3, 2020 • edited Loading

Codecov Report

CaselIT commented Apr 30, 2020

vytas7 commented Aug 4, 2020 • edited Loading

vytas7 commented Dec 16, 2020

CaselIT commented Dec 16, 2020 • edited Loading

kgriffs commented Dec 16, 2020

CaselIT commented Dec 16, 2020

CaselIT commented May 13, 2021

vytas7 commented May 14, 2021

CaselIT commented May 14, 2021

CaselIT commented May 14, 2021

CaselIT commented May 14, 2021 • edited Loading

vytas7 May 15, 2021

Choose a reason for hiding this comment

CaselIT May 15, 2021

Choose a reason for hiding this comment

CaselIT May 15, 2021 • edited Loading

Choose a reason for hiding this comment

vytas7 left a comment

Choose a reason for hiding this comment

CaselIT commented May 20, 2021 • edited Loading

vytas7 left a comment

Choose a reason for hiding this comment

kgriffs left a comment

Choose a reason for hiding this comment

CaselIT commented May 25, 2021

CaselIT commented May 26, 2021

kgriffs left a comment

Choose a reason for hiding this comment

vytas7 left a comment

Choose a reason for hiding this comment

CaselIT commented Apr 3, 2020 •

edited

Loading

codecov bot commented Apr 3, 2020 •

edited

Loading

vytas7 commented Aug 4, 2020 •

edited

Loading

CaselIT commented Dec 16, 2020 •

edited

Loading

CaselIT commented May 14, 2021 •

edited

Loading

CaselIT May 15, 2021 •

edited

Loading

CaselIT commented May 20, 2021 •

edited

Loading