search

imr-framework / pypulseq

Pulseq in Python
https://pypulseq.readthedocs.io
GNU Affero General Public License v3.0
125 stars 67 forks source link

Replace setup.py by pyproject.toml #195

Closed btasdelen closed 3 months ago

btasdelen commented 3 months ago

This PR aims to replace deprecated setup.py with a "pyproject.toml", hopefully without breaking anything. Relevant discussion #124. Luckily, setup.py was simple enough to easily replace. Alas, PR requires some input in regard to the following points:

  1. I could not figure out how to use "version.py" file to fetch current version dynamically. There are of course ways to dynamically do it: https://setuptools.pypa.io/en/stable/userguide/pyproject_config.html, but our directory structure is not standard, and version file does not follow module attr, so I do not know how to set dynamic version attr.

  2. I need help testing this plays well with uploading to PyPI.

FrankZijlstra commented 3 months ago

We could probably move the version information into the top level __init__.py, and provide a __version__ variable computed from the major/minor/revision variables. Then in sequence.py just from pypulseq import major, minor, revision should work?

btasdelen commented 3 months ago

Putting the version into __init__.py does not work because there are imports (i.e. numpy) that are not available during the build time. Instead, I moved the version.py into the module and hopefully updated the relevant imports.

schuenke commented 3 months ago

Okay, 1st of all: sorry for the mess. I just wanted to include the pytest config, but that was a bit more tricky than expected.

But now everything works, including building the package (locally and in our CI/CD pipeline). This means it's also PyPi compatabible.

My formatter did a lot of changes in sequence.py, so that I manually selected the important lines for the commit. That worked fine, but for some reason GitHub shows that the whole file was changed. It was not. Here is my local diff of sequence.py before / after my changes:

grafik

Regarding the version number: defining it in the pyproject toml and than using this info everywhere else in the package employing for example importlib.metadata is not super super nice, but actually the way other big packages like scipy / numpy etc do it at the moment as well until PEP 639 is accepted. See https://github.com/scipy/scipy/blob/4d2c9e050663efd54396a3d8749792fad0b55dde/pyproject.toml#L36-L38

EDIT: In the last commit I forgot to manually select the relevant lines. This is why some formatting is included as well. Sorry for that. But maybe this is a good example why we should define formatting standards as soon as possible. I will create an Issue for that and create a PR when we agree on something. Spoiler: I am a big fan of RUFF

btasdelen commented 3 months ago

@schuenke Looks good, but two concerns:

  1. Are you sure MANIFEST.in is deprecated? We are not using it right now, but setuptools describes its usage: https://setuptools.pypa.io/en/stable/userguide/datafiles.html

  2. Setting the license using classifiers rather than the license key was the recommended method. See: https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#writing-pyproject-toml

schuenke commented 3 months ago

I will check both points tomorrow again.

schuenke commented 3 months ago

Regarding point 1:

The package_data argument is a dictionary that maps from package names to lists of glob patterns. Note that the data files specified using the package_data option neither require to be included within a MANIFEST.in file, nor require to be added by a revision control system plugin.

Regarding point 2:

The advantage is, that the license is defined in a single position only, which helps to prevent inconsistencies when changing something. But I don't have a super strong opinion here and would be fine with adding it in the classifiers as well.

btasdelen commented 3 months ago

Sounds good. Should be ready for merging if there are no other concerns.