PEP-517 and PEP-518 support (add pyproject.toml) #274

groodt · 2021-01-10T10:09:48Z

hnswlib does not use the recommended packaging approach for pybind11.
https://pybind11.readthedocs.io/en/stable/compiling.html#setup-helpers-pep518

The pyproject.toml file can specify the requirements necessary for the build backend (setuptools in this case) that are installed before the actual build takes place.

This should also make it easier to start packaging wheels etc if this project moves to a more modern packaging approach.

.gitignore

.travis.yml

setup.py

yurymalkov

Cool! Thanks!
I wonder, will the current script (https://github.com/nmslib/hnswlib/blob/develop/Makefile) work for publishing pip packages?

setup.py

groodt · 2021-01-11T07:45:50Z

I wonder, will the current script (https://github.com/nmslib/hnswlib/blob/develop/Makefile) work for publishing pip packages?

Yes, in this PR, the project will still publish an "sdist" (source distribution), not a "wheel" (binary distribution). However, now the "sdist" will be built correctly by consumers of this package without having to manually install pip install pybind11 numpy setuptools into their environment beforehand when the legacy fallback behavior of pip is removed.

Compare what happens with pip install . of the develop branch and pip install hnswlib==0.4.0:

Using legacy setup.py install for hnswlib, since package 'wheel' is not installed.

This message no longer appears if you try the install into a clean virtualenv of this branch.

You will see the PEP-517 behavior kick in:

**Building wheels for collected packages: hnswlib
  Building wheel for hnswlib (PEP 517) ... done**

yurymalkov · 2021-01-11T22:48:36Z

Got it! Thanks! I'll check it and merge. @dyashuni can you please also check?

dyashuni · 2021-01-12T05:13:49Z

Sure, I'll check it.

dyashuni · 2021-01-13T12:17:10Z

Currently in the travis we build hnswlib twice. When we run python -m pip install at an installation phase and python setup.py test at a test phase. I wonder why python setup.py test rebuilds the lib if it is installed.
Link to travis logs: https://travis-ci.org/github/nmslib/hnswlib/jobs/753778781

groodt · 2021-01-13T20:52:17Z

Currently in the travis we build hnswlib twice. When we run python -m pip install at an installation phase and python setup.py test at a test phase. I wonder why python setup.py test rebuilds the lib if it is installed.
Link to travis logs: https://travis-ci.org/github/nmslib/hnswlib/jobs/753778781

Yes. I didn't really want to mention it, but setup.py test is also deprecated: pypa/setuptools#1878

I've made some changes to fix this in the latest commit. See: 467c98f

Essentially, this project can do with a bit of refactoring in the way that it is packaged and the Python coding conventions to make it follow best-practices a bit.

I can raise a few more PRs to propose some changes in future if I find some time and if the project is interested.

setup.py

.travis.yml

python_bindings/tests/bindings_test.py

python_bindings/tests/bindings_test_labels.py

yurymalkov

Looks good to me!

dyashuni · 2021-01-14T14:11:23Z

Good changes, thank you very much @groodt!

dyashuni · 2021-01-14T14:49:32Z

One thing to mention, now in Makefile python3 setup.py sdist doesn't work if pybind11 is not installed.

What is the best way to handle it @groodt ? Add an additional target in Makefile that will allow a user to install all required packages to build sdist, for example, pip3 install pybind11 numpy setuptools ?

groodt · 2021-01-15T03:58:18Z

One thing to mention, now in Makefile python3 setup.py sdist doesn't work if pybind11 is not installed.

Ah, you are correct. My apologies. I should have checked. Yes, these new PEP standards do encourage more publishing of wheels than sdist. We don't need to make this change now (or ever).

At least this inconvenience is only for the package uploader to PyPI, which is presumably a limited number of people.

There is an official PEP517 builder that supports both sdist and wheels, see: https://github.com/pypa/build

I'll make this change to the Makefile shortly.

README.md

dyashuni · 2021-01-15T15:55:00Z

python3 setup.py sdist

It fixes the issue, thank you @groodt !

dyashuni

@groodt Thank you!

yurymalkov

Looks great! Do you plan add something to this PR?

groodt · 2021-01-17T05:46:10Z

Nope. I would be happy for it to be merged.

…

On Sun, 17 Jan 2021 at 14:35, Yury Malkov ***@***.***> wrote: ***@***.**** approved this pull request. Looks great! Do you plan add something to this PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#274 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACT254PT5SM6CVG4K3C3BLS2JLJRANCNFSM4V4JWWIA> .

yurymalkov · 2021-01-17T07:14:55Z

Cool! Thank you so much!
Also thanks to @dyashuni for reviewing!

groodt · 2021-01-17T21:35:17Z

Thanks for the reviews @yurymalkov @dyashuni

Is there likely to be a new release to PyPI anytime soon or are there more features waiting to land on develop?

yurymalkov · 2021-01-17T22:08:49Z

Yes, we plan to release soon, there are significant changes.
Most likely, the following week.

groodt · 2021-01-28T07:14:40Z

@yurymalkov No rush, just checking in, but any news on the next release date?

yurymalkov · 2021-01-29T04:59:04Z

@groodt
Done! Thanks!

groodt · 2021-01-29T21:34:12Z

@groodt
Done! Thanks!

Awesome! Thanks again!

PEP-517 support

68b6257

groodt changed the base branch from master to develop January 10, 2021 10:10

This was referenced Jan 10, 2021

Python install error #269

Closed

No pyproject.toml #177

Open

Simplify include_dirs

e94c5dc

groodt commented Jan 10, 2021

View reviewed changes

.gitignore Show resolved Hide resolved

groodt commented Jan 10, 2021

View reviewed changes

.travis.yml Show resolved Hide resolved

groodt commented Jan 10, 2021

View reviewed changes

setup.py Outdated Show resolved Hide resolved

groodt commented Jan 10, 2021

View reviewed changes

setup.py Show resolved Hide resolved

groodt changed the title ~~PEP-517 support (add pyproject.toml)~~ PEP-517 and PEP-518 support (add pyproject.toml) Jan 10, 2021

yurymalkov reviewed Jan 11, 2021

View reviewed changes

setup.py Show resolved Hide resolved

Remove deprecated setup.py test

467c98f

pybind11 isn't needed at runtime, only build time

2248ab4

groodt commented Jan 13, 2021

View reviewed changes

setup.py Show resolved Hide resolved

groodt commented Jan 13, 2021

View reviewed changes

setup.py Show resolved Hide resolved

setup.py Show resolved Hide resolved

.travis.yml Show resolved Hide resolved

python_bindings/tests/bindings_test.py Show resolved Hide resolved

python_bindings/tests/bindings_test_labels.py Show resolved Hide resolved

groodt requested a review from yurymalkov January 13, 2021 21:23

yurymalkov approved these changes Jan 14, 2021

View reviewed changes

dyashuni approved these changes Jan 14, 2021

View reviewed changes

Support for packaging sdist

8fe02c0

groodt requested a review from yurymalkov January 15, 2021 04:05

dyashuni reviewed Jan 15, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

https git clone in README

73134a7

dyashuni approved these changes Jan 16, 2021

View reviewed changes

yurymalkov approved these changes Jan 17, 2021

View reviewed changes

yurymalkov merged commit 65a5f28 into nmslib:develop Jan 17, 2021

groodt deleted the groodt-pyproject-toml branch January 17, 2021 21:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PEP-517 and PEP-518 support (add pyproject.toml) #274

PEP-517 and PEP-518 support (add pyproject.toml) #274

groodt commented Jan 10, 2021 •

edited

Loading

yurymalkov left a comment

groodt commented Jan 11, 2021 •

edited

Loading

yurymalkov commented Jan 11, 2021

dyashuni commented Jan 12, 2021

dyashuni commented Jan 13, 2021 •

edited

Loading

groodt commented Jan 13, 2021 •

edited

Loading

yurymalkov left a comment

dyashuni commented Jan 14, 2021

dyashuni commented Jan 14, 2021

groodt commented Jan 15, 2021 •

edited

Loading

dyashuni commented Jan 15, 2021

dyashuni left a comment

yurymalkov left a comment

groodt commented Jan 17, 2021 via email

yurymalkov commented Jan 17, 2021

groodt commented Jan 17, 2021

yurymalkov commented Jan 17, 2021

groodt commented Jan 28, 2021

yurymalkov commented Jan 29, 2021

groodt commented Jan 29, 2021

PEP-517 and PEP-518 support (add pyproject.toml) #274

PEP-517 and PEP-518 support (add pyproject.toml) #274

Conversation

groodt commented Jan 10, 2021 • edited Loading

yurymalkov left a comment

Choose a reason for hiding this comment

groodt commented Jan 11, 2021 • edited Loading

yurymalkov commented Jan 11, 2021

dyashuni commented Jan 12, 2021

dyashuni commented Jan 13, 2021 • edited Loading

groodt commented Jan 13, 2021 • edited Loading

yurymalkov left a comment

Choose a reason for hiding this comment

dyashuni commented Jan 14, 2021

dyashuni commented Jan 14, 2021

groodt commented Jan 15, 2021 • edited Loading

dyashuni commented Jan 15, 2021

dyashuni left a comment

Choose a reason for hiding this comment

yurymalkov left a comment

Choose a reason for hiding this comment

groodt commented Jan 17, 2021 via email

yurymalkov commented Jan 17, 2021

groodt commented Jan 17, 2021

yurymalkov commented Jan 17, 2021

groodt commented Jan 28, 2021

yurymalkov commented Jan 29, 2021

groodt commented Jan 29, 2021

groodt commented Jan 10, 2021 •

edited

Loading

groodt commented Jan 11, 2021 •

edited

Loading

dyashuni commented Jan 13, 2021 •

edited

Loading

groodt commented Jan 13, 2021 •

edited

Loading

groodt commented Jan 15, 2021 •

edited

Loading