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

PyProject-centric Build System with Setuptools #332

Merged
merged 13 commits into from
May 13, 2023
Merged

PyProject-centric Build System with Setuptools #332

merged 13 commits into from
May 13, 2023

Conversation

ludgerpaehler
Copy link
Contributor

This PR fully converts the existing setup tools-centric build system to a pyproject.toml-centric build system with Poetry as a part of this the PR introduces the following changes:

  • Removal of setup.cfg with the flake8 configuration going its own dedicated .flake8 configuration file
  • Splitting up requirements-dev.txt into a dev dependency group, a docs dependency group to solely build the documentation, and moving cvxpy into the normal dependencies. If desired, cvxpy could be broken out into its own separate dependency group. These can be installed with pip install pysindy[dev], or pip install pysindy[docs].
  • The workflows have been changed to accommodate the new build system
  • The content of setup.py has been carried over into the pyproject.toml
  • The README has been adjusted to the changes, and changed where necessary.

@Jacob-Stevens-Haas
Copy link
Member

Jacob-Stevens-Haas commented May 10, 2023

A PR to move from setup.py, requirements.txt, and setup.cfg to a pyproject.toml build has long been on the backburner, so a HUGE thanks for getting it started!

That said, I'm more inclined to setuptools_scm than poetry as a backend, for a few reasons:

  • It keeps metadata specification as in PEP 621 and PEP 631.
  • It auto-assigns version number from git tag (there is a 3rd party wrapper around poetry backend as an alternate, however)

Also, I'm not sure that committing a lockfile is appropriate, as we want new versions of dependencies to cause CI failures, as that forces us to keep the library compatible with a wider range of dependencies. I know some people have a workflow for package management that uses a lockfile, but I've never really understood them.

Was there some discussion about poetry that I missed?

@ludgerpaehler
Copy link
Contributor Author

@Jacob-Stevens-Haas are you available to quickly triage the differences today/tomorrow? Started it during your talk on Monday, and just finished the first version yesterday.

@Jacob-Stevens-Haas Jacob-Stevens-Haas self-assigned this May 10, 2023
@ludgerpaehler
Copy link
Contributor Author

No personal preference between setuptools, and poetry tbh - am just more used to poetry so that is where I started, but happy to convert everything to setuptools, and utilize setuptools_scm.

As far as I can tell, the lock-file should be excluded from the PR - but if it is hiding somewhere, then I'll gladly rewrite the Git history to remove it.

I think the files where we already have full alignment are, and please correct me if I am wrong:

  • .flake8
  • pre-commit-config.yaml
  • .github/workflows/main.yml
  • README.rst

With most of the others being dependent on the build system used by pyproject.toml, and hence being blocked by the switch of the build system to setuptools.

What is your opinion of how the packages are split up into the three groups with the normal installation with cvxpy, dev, and docs? Should packages be moved into different groups?

While letting Poetry determine the version constraints, I also managed to remove duplicate package quotations inside of the requirements, and the dev-requirements which are already installed due to other packages.

Copy link
Member

@Jacob-Stevens-Haas Jacob-Stevens-Haas left a comment

Choose a reason for hiding this comment

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

Thanks again! Most comments are about keeping the setuptools backend and setuptools_scm plugin, and how that changes pyproject.toml. Currently an open issue to extend poetry support to PEP-defined format.

Thanks also for the documentation upgrade and removing duplicate packages!

What is your opinion of how the packages are split up into the three groups with the normal installation with cvxpy, dev, and docs? Should packages be moved into different groups?

I think I see what you mean with pip install pysindy[cvxpy]. Only concern is that, as an optional dependency, we need to guard imports from cvxpy. We tried to do that with pip install pysindy[miosr], but I'm not sure we were truly able to do it correctly. I'm interested to hear whether @znicolaou and @akaptano think that cvxpy is a difficult enough requirement to justify putting it in its own optional dependency. FWIW, MIOSR/gurobipy, requires a paid (or academic) license to use the full functionality.

.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@ludgerpaehler ludgerpaehler changed the title PyProject-centric Build System with Poetry PyProject-centric Build System with Setuptools May 12, 2023
@codecov-commenter
Copy link

codecov-commenter commented May 12, 2023

Codecov Report

Patch coverage has no change and project coverage change: -92.32 ⚠️

Comparison is base (44f04ab) 92.31% compared to head (c8d640a) 0.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #332       +/-   ##
==========================================
- Coverage   92.31%   0.00%   -92.32%     
==========================================
  Files          37      37               
  Lines        3747    3747               
==========================================
- Hits         3459       0     -3459     
- Misses        288    3747     +3459     

see 37 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ludgerpaehler
Copy link
Contributor Author

@Jacob-Stevens-Haas now the PR should be ready to merge :) The Readme is adjusted, and should be good-to-go.

Thanks for sitting down with me!

@Jacob-Stevens-Haas
Copy link
Member

Jacob-Stevens-Haas commented May 13, 2023

Thanks @ludgerpaehler ! I noticed that for some reason, codecov shows:

Patch coverage has no change and project coverage change: -92.32 warning

which is odd, because we didn't change anything with pytest-coverage or codecov. I'm going to merge it now and troubleshoot over the weekend, since codecov has been going through some weird changes recently. Do you remember anything we did that could've caused this?

In the CI logs:

/opt/hostedtoolcache/Python/3.10.11/x64/lib/python3.10/site-packages/coverage/control.py:858: CoverageWarning: No data was collected. (no-data-collected)

@Jacob-Stevens-Haas Jacob-Stevens-Haas merged commit ebc1984 into dynamicslab:master May 13, 2023
jpcurbelo pushed a commit to jpcurbelo/pysindy_fork that referenced this pull request Apr 30, 2024
PyProject-centric Build System with Setuptools
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