search

amitdev / lru-dict

A fast and memory efficient LRU cache for Python
MIT License
260 stars 46 forks source link

:construction_worker: Wheels build, refs #23 #38

Closed AndreMiras closed 2 years ago

AndreMiras commented 2 years ago

Leverages cibuildwheel to build wheels for different platforms and Python versions then upload them as GitHub Artifact. Note that it significantly increases the build time from instant to about 5-8 minutes. Follow up pull request will use the artifacts to publish to PyPI, refs: https://cibuildwheel.readthedocs.io/en/stable/deliver-to-pypi/

Erotemic commented 2 years ago

It might be good to configure cibuildwheel with a file pyproject.toml. The build time can be brought down only building wheels for supported versions of Python.

AndreMiras commented 2 years ago

Hey @Erotemic thanks for looking this up and giving feedback. What versions do you have in mind? I think it currently builds for Python 3.6 up to 3.10 which is what lru-dict is currently supporting (minus 2.7 which is a good thing. See the artifact extract below:

$ unzip -l artifact.zip 
Archive:  artifact.zip
  Length      Date    Time    Name
---------  ---------- -----   ----
    10794  2022-06-25 19:10   lru-dict-1.1.7.tar.gz
     9832  2022-06-25 19:16   lru_dict-1.1.7-cp310-cp310-macosx_10_9_x86_64.whl
    27326  2022-06-25 19:17   lru_dict-1.1.7-cp310-cp310-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl
    28862  2022-06-25 19:17   lru_dict-1.1.7-cp310-cp310-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_17_x86_64.manylinux2014_x86_64.whl
    32062  2022-06-25 19:17   lru_dict-1.1.7-cp310-cp310-musllinux_1_1_i686.whl
    33194  2022-06-25 19:17   lru_dict-1.1.7-cp310-cp310-musllinux_1_1_x86_64.whl
    12213  2022-06-25 19:19   lru_dict-1.1.7-cp310-cp310-win_amd64.whl
    11239  2022-06-25 19:19   lru_dict-1.1.7-cp310-cp310-win32.whl
     9784  2022-06-25 19:16   lru_dict-1.1.7-cp36-cp36m-macosx_10_9_x86_64.whl
    25509  2022-06-25 19:17   lru_dict-1.1.7-cp36-cp36m-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl
    26793  2022-06-25 19:17   lru_dict-1.1.7-cp36-cp36m-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_17_x86_64.manylinux2014_x86_64.whl
    30278  2022-06-25 19:17   lru_dict-1.1.7-cp36-cp36m-musllinux_1_1_i686.whl
    31174  2022-06-25 19:17   lru_dict-1.1.7-cp36-cp36m-musllinux_1_1_x86_64.whl
    12736  2022-06-25 19:19   lru_dict-1.1.7-cp36-cp36m-win_amd64.whl
    11503  2022-06-25 19:19   lru_dict-1.1.7-cp36-cp36m-win32.whl
     9772  2022-06-25 19:16   lru_dict-1.1.7-cp37-cp37m-macosx_10_9_x86_64.whl
    25487  2022-06-25 19:17   lru_dict-1.1.7-cp37-cp37m-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl
    26773  2022-06-25 19:17   lru_dict-1.1.7-cp37-cp37m-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_17_x86_64.manylinux2014_x86_64.whl
    31199  2022-06-25 19:17   lru_dict-1.1.7-cp37-cp37m-musllinux_1_1_i686.whl
    32168  2022-06-25 19:17   lru_dict-1.1.7-cp37-cp37m-musllinux_1_1_x86_64.whl
    12278  2022-06-25 19:19   lru_dict-1.1.7-cp37-cp37m-win_amd64.whl
    11234  2022-06-25 19:19   lru_dict-1.1.7-cp37-cp37m-win32.whl
     9800  2022-06-25 19:16   lru_dict-1.1.7-cp38-cp38-macosx_10_9_x86_64.whl
    27943  2022-06-25 19:17   lru_dict-1.1.7-cp38-cp38-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl
    29404  2022-06-25 19:17   lru_dict-1.1.7-cp38-cp38-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_17_x86_64.manylinux2014_x86_64.whl
    32323  2022-06-25 19:17   lru_dict-1.1.7-cp38-cp38-musllinux_1_1_i686.whl
    33470  2022-06-25 19:17   lru_dict-1.1.7-cp38-cp38-musllinux_1_1_x86_64.whl
    12268  2022-06-25 19:19   lru_dict-1.1.7-cp38-cp38-win_amd64.whl
    11294  2022-06-25 19:19   lru_dict-1.1.7-cp38-cp38-win32.whl
     9833  2022-06-25 19:16   lru_dict-1.1.7-cp39-cp39-macosx_10_9_x86_64.whl
    26904  2022-06-25 19:17   lru_dict-1.1.7-cp39-cp39-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl
    28434  2022-06-25 19:17   lru_dict-1.1.7-cp39-cp39-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_17_x86_64.manylinux2014_x86_64.whl
    31661  2022-06-25 19:17   lru_dict-1.1.7-cp39-cp39-musllinux_1_1_i686.whl
    32764  2022-06-25 19:17   lru_dict-1.1.7-cp39-cp39-musllinux_1_1_x86_64.whl
    12258  2022-06-25 19:19   lru_dict-1.1.7-cp39-cp39-win_amd64.whl
    11271  2022-06-25 19:19   lru_dict-1.1.7-cp39-cp39-win32.whl
     8866  2022-06-25 19:16   lru_dict-1.1.7-pp37-pypy37_pp73-macosx_10_9_x86_64.whl
    11851  2022-06-25 19:17   lru_dict-1.1.7-pp37-pypy37_pp73-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl
    11552  2022-06-25 19:17   lru_dict-1.1.7-pp37-pypy37_pp73-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_17_x86_64.manylinux2014_x86_64.whl
    12387  2022-06-25 19:19   lru_dict-1.1.7-pp37-pypy37_pp73-win_amd64.whl
     8865  2022-06-25 19:16   lru_dict-1.1.7-pp38-pypy38_pp73-macosx_10_9_x86_64.whl
    11853  2022-06-25 19:17   lru_dict-1.1.7-pp38-pypy38_pp73-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl
    11555  2022-06-25 19:17   lru_dict-1.1.7-pp38-pypy38_pp73-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_17_x86_64.manylinux2014_x86_64.whl
    12387  2022-06-25 19:19   lru_dict-1.1.7-pp38-pypy38_pp73-win_amd64.whl
     8869  2022-06-25 19:16   lru_dict-1.1.7-pp39-pypy39_pp73-macosx_10_9_x86_64.whl
    11859  2022-06-25 19:17   lru_dict-1.1.7-pp39-pypy39_pp73-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl
    11538  2022-06-25 19:17   lru_dict-1.1.7-pp39-pypy39_pp73-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_17_x86_64.manylinux2014_x86_64.whl
    12386  2022-06-25 19:19   lru_dict-1.1.7-pp39-pypy39_pp73-win_amd64.whl
---------                     -------
   905805                     48 files
$

What is taking the most is the Windows build. I left the default cibuildwheel options to keep it simple and maybe we could iterate/tune later, but I don't know if lru-dict actually has Windows or macOS users. That being said the project seems to be in maintenance mode with a very few commits lately, so 8 minutes of build time shouldn't hurt too much I hope

Erotemic commented 2 years ago

Ah, those were the versions that I had in mind. Looking at setup.py I saw: 

        'Programming Language :: Python :: 2',
        'Programming Language :: Python :: 3',
        'Programming Language :: Python :: Implementation :: CPython',

which made me think anything from 2.7-3.5 might be in there, which I'm glad it's not.

It might be good to move some of that metadata from setup.py into the static pyproject.toml file, and then update it to reflect the versions that are actually supported. (including pypy). (Or just update the values in setup.py would be a fine compromise).

I agree 8 minutes isn't a bad build time. And I think supporting OSX and Win32 is important as long as it isn't too much of a headache (I only drop support for them when they present insurmountable issues).

Also, for pypy, the line open('README.rst').read() might break. Last time I checked pypy has an issue where if you don't assign the result of open to a variable: https://www.pypy.org/compat.html This can be fixed by either using a context manager, or you could do something like import pathlib and pathlib.Path('README.rst').read_text(). However, it's unlikely the setup.py script will ever get into a state that will trigger that bug, it's just something I'm aware of when I use pypy.

AndreMiras commented 2 years ago

Nice feedback thanks for that. Definitely agree, I thought of maybe maintaining the setup.py a bit indeed, but I thought let's keep it for a dedicated pull request. I tend to make small iterations because I feel it gets higher chances of getting merged quickly. I didn't know about pypy and the open('README.rst').read() notation. Best would be to also run tests under pypy to find out (including a local install test), I think there's definitely value in that too. A good start could be to make a dedicated pull request running tests with pypy, I can look into that too Edit: Here it is https://github.com/amitdev/lru-dict/pull/39

Erotemic commented 2 years ago

FYI: cibuildwheel has a CIBW_TEST_COMMAND option: https://cibuildwheel.readthedocs.io/en/stable/options/#test-command that can be used to ensure tests are executed on all artifacts.

AndreMiras commented 2 years ago

Oh thanks for that, I gave it a quick try, but it didn't workout well because it means we need to ship the tests in the wheels (so I reverted the change). This is probably OK to do usually, but kind of increase the scope of what I'm addressing. I prefer not to rethink the way this project is done in this pull request. Let's see if that iteration gets approved/merged and keep going as we see fit.

Erotemic commented 2 years ago

Sounds good to me. Best not to overthink this. The minimal path to get binary wheels on pypi should be sufficient.