Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: update package metadata #3079

Merged
merged 4 commits into from
Aug 24, 2022
Merged

Conversation

ofek
Copy link
Contributor

@ofek ofek commented Aug 21, 2022

Background

Hello there! The Python packaging ecosystem has standardized on the interface for build backends (PEP 517/PEP 660) and the format for metadata declaration (PEP 621/PEP 631). As a result, the execution of setup.py files is now deprecated.

So, I'm spending my free time updating important projects so that they are modernized and set an example for others 😄

Proposed Changes

This implements PEP 621, obviating the need for setup.py and setup.cfg. The build backend hatchling (of which I am a maintainer in the PyPA) is now used as that is the default in the official Python packaging tutorial. Hatchling is available on all the major distribution channels such as Debian, Fedora, Arch Linux, conda-forge, Nixpkgs, Alpine Linux, FreeBSD/OpenBSD, Gentoo Linux, MacPorts, OpenEmbedded, Spack, etc.

How did you test it?

tox -e py37

Notes for the reviewer

  • The source distributions on PyPI are erroneously shipping a build artifact *.egg-info from python setup.py develop; this is now fixed
  • I made it possible to test with tox again
  • We should probably remove the Python upper bound constraint as upper bounds for libraries are heavily discouraged by the community, see https://iscinumpy.dev/post/bound-version-constraints/

Checklist

@ofek ofek requested review from a team as code owners August 21, 2022 14:07
@ofek ofek requested review from bogdankostic and removed request for a team August 21, 2022 14:08
@CLAassistant
Copy link

CLAassistant commented Aug 21, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@masci masci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a ton Ofek, this is great!

Ok for me to remove the capping for Python

.github/actions/python_cache/action.yml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
COVERAGE_FILE = test-reports/.coverage
PYTEST_ADDOPTS = --junitxml=test-reports/{envname}/junit.xml -vv
commands =
coverage run --source haystack --parallel-mode -m pytest {posargs}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@masci masci added type:refactor Not necessarily visible to the users topic:tests topic:build/distribution labels Aug 21, 2022
@masci masci added this to the 1.8.0 milestone Aug 21, 2022
Copy link
Contributor

@masci masci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for someone else from @deepset-ai/core-engineering to also review before merging but I'm super happy about this change, LGTM!

@masci masci requested a review from ZanSara August 21, 2022 21:26
Copy link
Contributor

@ZanSara ZanSara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks a ton for this change!! 🎉

I found one small change to do in the list of files and folders we package, but other than that it's ready to go for me. Thanks again!

pyproject.toml Outdated
Comment on lines 245 to 249
include = [
"/haystack",
"/rest_api",
"/VERSION.txt",
]
Copy link
Contributor

@ZanSara ZanSara Aug 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to ship the REST API with the package (probably now they're shipped even if they shouldn't). On the other hand, we should rather include LICENSE?

Suggested change
include = [
"/haystack",
"/rest_api",
"/VERSION.txt",
]
include = [
"/haystack",
"/LICENSE",
"/VERSION.txt",
]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Licenses are always included https://hatch.pypa.io/latest/plugins/builder/sdist/#default-file-selection

Removed! Yes I was following sdist on PyPI:

hs

Copy link
Contributor

@ZanSara ZanSara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you once more! 😊

@ofek
Copy link
Contributor Author

ofek commented Aug 22, 2022

Happy to help! Cool project btw 🥇

@ZanSara ZanSara changed the title Update package metadata refactor: update package metadata Aug 24, 2022
@ZanSara ZanSara merged commit f6a4a14 into deepset-ai:main Aug 24, 2022
@ofek ofek deleted the modernize-metadata branch August 24, 2022 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants