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 code refactor #289

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

sfarrens
Copy link
Contributor

Summary

This PR refactors the codebase to simplify maintenance and development.

  • Removed setup.py and setup.cfg and added pyproject.toml
  • Removed redundant files/directories: .pylintrc, .pyup.yml, develop.txt, docs/requirements.txt, notebooks
  • Ran Black on modopt (i.e. only style changes, no changes to code or tests)
  • Updated docs: removed redundant notebook tools, cleaned up
  • Updated manifest
  • Simplified CI tests: several tests can now be run manually rather that being triggered for each commit, only the latest version of Python will run automatically

To Do

  • Update README.md and installation instructions
  • Check that new CI/CD works correctly
  • Additional checks

@sfarrens sfarrens added the clean up clean up of small typos etc. label Mar 16, 2023
@sfarrens sfarrens self-assigned this Mar 16, 2023
@sfarrens sfarrens added this to ToDo in ModOpt-dev via automation Mar 16, 2023
@sfarrens sfarrens requested a review from paquiteau March 16, 2023 17:39
@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2023

Codecov Report

❗ No coverage uploaded for pull request base (develop@2d7da27). Click here to learn what that means.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             develop     #289   +/-   ##
==========================================
  Coverage           ?   91.48%           
==========================================
  Files              ?       44           
  Lines              ?     2372           
  Branches           ?        0           
==========================================
  Hits               ?     2170           
  Misses             ?      202           
  Partials           ?        0           
Flag Coverage Δ
unittests 91.48% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@paquiteau paquiteau left a comment

Choose a reason for hiding this comment

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

Great work !
We might want to push it further by using a src layout to tidy things up further and avoiding distributing test, examples and other files with the python package (see https://packaging.python.org/en/latest/discussions/src-layout-vs-flat-layout/ for more info)

Also, the signals for the CI needs to be update I guess in the github settings (to avoid the Expected — Waiting for status to be reported )

MANIFEST.in Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not so sure we need a MANIFEST.in (which is for source distribution) anymore. Everything should be handled by pyproject.toml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to find some concrete information on this, if you have any references for this point I will check them out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this issue again for another project of mine I found the following references:

TLDR: As we are in a python-only codebase, with no extra data files, there is no need for the MANIFEST.in everything is handled by setuptools.

)

__version__ = _version
__version__ = "1.6.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of hardcoding the version, use setuptools-scm plugin and the following

Suggested change
__version__ = "1.6.1"
try:
# -- Distribution mode --
# import from _version.py generated by setuptools_scm during release
from ._version import version as __version__
except ImportError:
# -- Source mode --
# use setuptools_scm to get the current version from src using git
from setuptools_scm import get_version as _gv
from os import path as _path
__version__ = _gv(_path.join(_path.dirname(__file__), _path.pardir))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this was always just temporary, but nice implementation! I will quick look at what the current consensus is on this point before committing, but I like this.

modopt/signal/svd.py Outdated Show resolved Hide resolved
Comment on lines +5 to +10
authors = [
{ "name" = "Samuel Farrens", "email" = "samuel.farrens@cea.fr"},
]
maintainers = [
{ "name" = "Samuel Farrens", "email" = "samuel.farrens@cea.fr" },
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should'nt be more people in the list 😇 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, of course! I am still playing around with the formatting and metadata. I was think for the authors section we could put something like "The CEA-COSMIC Team" or "The ModOpt Team" to be a more inclusive term for everyone who has contributed. Any thoughts/suggestions are certainly welcome.

I will explicitly add you as a maintainer before I finalise this PR.

pyproject.toml Outdated Show resolved Hide resolved
Comment on lines +94 to +96
[tool.setuptools.dynamic]
dependencies = {file = ["requirements.txt"]}
version = {attr = "modopt.__version__"}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather declare the dependencies directly in the pyproject.toml file.
And use setuptools-scm to track the version (see my other comment on modopt/__init__.py

Suggested change
[tool.setuptools.dynamic]
dependencies = {file = ["requirements.txt"]}
version = {attr = "modopt.__version__"}
[tool.setuptools_scm]
write_to = "modopt/_version.py"
version_scheme = "python-simplified-semver"
local_scheme="no-local-version"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will look into this.

sfarrens and others added 2 commits March 20, 2023 17:07
Co-authored-by: Pierre-Antoine Comby <pierre-antoine.comby@crans.org>
Co-authored-by: Pierre-Antoine Comby <pierre-antoine.comby@crans.org>
@sfarrens
Copy link
Contributor Author

Thanks for the feedback @paquiteau, I am still playing around with this PR, but I like these suggestions. I will play around some more this week and ping you when the PR is ready for final review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean up clean up of small typos etc.
Projects
ModOpt-dev
  
ToDo
Development

Successfully merging this pull request may close these issues.

None yet

3 participants