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

build: use PEP517/518 conventions #100

Merged
merged 80 commits into from
Jan 6, 2021
Merged

build: use PEP517/518 conventions #100

merged 80 commits into from
Jan 6, 2021

Conversation

deepcharles
Copy link
Owner

@deepcharles deepcharles commented Jan 2, 2021

  • move most content from setup.py to setup.cfg and pyproject.toml (except cython related extensions), following PEP517/518
  • clean gh actions names and commands
  • fix coverage upload to codecov

EDIT: see #100 (comment) for a summary of this PR.

@deepcharles
Copy link
Owner Author

deepcharles commented Jan 3, 2021

This pull requests implements several changes in the build and testing process.

Package layout

All package code is in a src/ directory now. Before, it was not clear which code was tested (the installed package or the script in the repo). See for instance here.

Code coverage

The code coverage now works as expected, meaning that the report uploaded to Codecov does not show 0% anymore.
The configuration options for pytest and pytest-cov are in pyproject.toml as it will be the future convention (see pytest documentation).

PEP 517/518

The biggest change deals with the build process. Following PEP 517 and PEP 518, the standartized way to package library is by using setup.cfg, pyproject.toml (and optionally setup.py). Basically, all that is needed to build the package is installed in a separate virtualenv, the package is then build (in a wheel format) and the virtualenv is removed. Build dependencies do not remain in the user's workspace any more.
To that end, I moved most of setup.py's content into setup.cfg. Infortunately, the cython extensions could not be adapted, so they remained in setup.py.
The requirements-dev.txt is no longer needed as it is included in setup.cfg (section [options.extras_require]). To build and install ruptures, one simply needs to run one of the following command (within the directory containing setup.cfg):

# for a normal installation
python -m pip install . 
# for a normal installation and matplotlib
python -m pip install .[display]
# for an editable installation
python -m pip install -e .
# to also install the test librairies
python -m pip install -e .[test]
# to also install the developpement librairies (for the documentation typically)
python -m pip install -e .[dev]

Note that the python -m can be omitted most of the times, but within virtualenvs, it can prevent certain bugs.
Also, in certain terminals (such as zsh), the square brackets must be escaped, e.g. replace .[test] by .\[test\].

The old way of building and installing (python setup.py install) will not work any more.

Github Actions

I modified the Github actions and the CircleCI config to reflect the previous changes. I also cleaned and formatted a bit (especially the titles).

Version number

Now the package version number is defined once, in src/ruptures/__init__.py.

@deepcharles
Copy link
Owner Author

deepcharles commented Jan 3, 2021

@oboulant I'll merge this PR in a couple of days so that we can discuss it a bit.

@deepcharles
Copy link
Owner Author

deepcharles commented Jan 3, 2021

Following the changes, the repo can be cleaned a bit:

  • remove README.rst
  • remove .flake8
  • update documentation to show the new way of installing an editable ruptures (in CONTRIBUTING.md)
  • MANIFEST.in contains useless instructions

@deepcharles
Copy link
Owner Author

On a side note, the package version number is now defined once, in src/ruptures/__init__.py.

src/ruptures/__init__.py Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
@oboulant oboulant self-requested a review January 4, 2021 15:56
Copy link
Collaborator

@oboulant oboulant left a comment

Choose a reason for hiding this comment

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

LGTM !

@deepcharles
Copy link
Owner Author

Merging now then!

@deepcharles deepcharles merged commit 577ea32 into master Jan 6, 2021
@deepcharles deepcharles deleted the better-setup branch January 6, 2021 15:12
@@ -1,7 +1,7 @@
[build-system]
# Minimum requirements for the build system to execute.
requires = [
"setuptools",
"setuptools>=38.3.0", # version with most `setup.cfg` bugfixes
Copy link

Choose a reason for hiding this comment

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

Tiny suggestion: I recommend >=42 for use in pyproject.toml. I spend about an hour once debugging a broken build, and it turned out I didn't specify a minimum, and it was grabbing a cached copy of 41.somthing instead of the latest version, which had critical bugs for PEP 517 builds. If you don't make a PEP 517 build, then this is ignored anyway, and if you are building PEP 517, you want 42+ and can always get it. ;) See https://scikit-hep.org/developer/packaging#pep-517518-support-high-priority

PS: Great work on the NumPy requirements, that's what I came here to look at. :)

Copy link

Choose a reason for hiding this comment

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

It might have been 40.something, but whatever it was the devs told me to go with 42+.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks, we will make the change then!

Copy link
Owner Author

Choose a reason for hiding this comment

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

See https://scikit-hep.org/developer/packaging#pep-517518-support-high-priority

Nice read!

PS: Great work on the NumPy requirements, that's what I came here to look at. :)

Many thanks! I

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants