jfowkes / pycutest

Python interface to CUTEst
https://jfowkes.github.io/pycutest/
GNU General Public License v3.0
29 stars 11 forks source link

Switch to pyproject.toml #77

Closed jfowkes closed 7 months ago

jfowkes commented 8 months ago

Resolves #76

This PR switches PyCUTEst over to the modern pyproject.toml way of installing things since using setup.py for installation is deprecated and Python 12 has stopped shipping with setuptools installed by default like it used to.

This closely follows the upstream best-practice recommendations here: https://packaging.python.org/en/latest/guides/writing-pyproject-toml/

As well as adding in the setuptools packages keyword as per: https://setuptools.pypa.io/en/latest/userguide/pyproject_config.html since the automatic discovery gets confused by the presence of CUTEst.

In order to completely remove setup.py the version number has been moved into __init__.py and is sourced where needed using from pycutest import __version__ as per: https://packaging.python.org/en/latest/guides/single-sourcing-package-version/ (pyproject.toml uses tool.setuptools.dynamic to read the version from __init.py__).

jfowkes commented 8 months ago

@ragonneau how does this look to you?

jfowkes commented 8 months ago

@lindonroberts an attempt to modernise the PyCUTEst install, would appreciate your thoughts and suggestions on this when you have some time (no hurry on this one).

ragonneau commented 8 months ago

@ragonneau how does this look to you?

Hi @jfowkes,

I am sorry I did not get the time to do this PR before you did.

Concerning the versioning of PyCUTEst, I suppose a cleaner version would be to define a __version__ variable in pycutest/__init__.py, and to then use this in pyproject.toml as:

[project]
...
dynamic = ["version"]

[tool.setuptools.dynamic]
version = {attr = "pycutest.__version__"}

[tool.setuptools.packages.find]
include = ["pycutest*"]

This is for example what I did for COBYQA. Using this technique, you do not need to rely on importlib to get the version number. What do you think?

ragonneau commented 8 months ago

Also, I suppose that this PR does not resolve the setuptools issue for Python 3.12, as the packages generated when compiling CUTEst problems still have setup.py files. If I understand correctly, I suppose that pycutest/build_interface.py should be updated to generate pyproject.toml files in lieu of setup.py. Did I misunderstand something?

Cheers, Tom.

jfowkes commented 8 months ago

I am sorry I did not get the time to do this PR before you did.

No worries at all @ragonneau, many thanks for your review!

Concerning the versioning of PyCUTEst, I suppose a cleaner version would be to define a __version__ variable in pycutest/__init__.py, and to then use this in pyproject.toml as:

[project]
...
dynamic = ["version"]

[tool.setuptools.dynamic]
version = {attr = "pycutest.__version__"}

[tool.setuptools.packages.find]
include = ["pycutest*"]

This is for example what I did for COBYQA. Using this technique, you do not need to rely on importlib to get the version number. What do you think?

Yeah that's also a good approach, I'd be happy with either. @lindonroberts do you have a preference of one versioning approach over the other? In this case we wouldn't need the tool.setuptools.packages.find entry as we already have

[tool.setuptools]
packages = ["pycutest"]

which tells setuptools to skip the automatic discovery and just use the pycutest package (this is cleaner imho).

Also, I suppose that this PR does not resolve the setuptools issue for Python 3.12, as the packages generated when compiling CUTEst problems still have setup.py files. If I understand correctly, I suppose that pycutest/build_interface.py should be updated to generate pyproject.toml files in lieu of setup.py. Did I misunderstand something?

It does actually as setuptools is now a requirement in the project dependencies in pyproject.toml and gets installed automatically (otherwise it would indeed break on Python 3.12 as I found out when testing this PR). But you do make a good point: should we also update pycutest/build_interface.py to generate pyproject.toml files in lieu of setup.py? I would say no as these are internal setuptools extensions and not Python packages per se (and setup.py itself is not deprecated as per the official docs).

lindonroberts commented 8 months ago

Hi @jfowkes

Thanks @ragonneau for setting this in motion! Regarding how to reference the package version, I don't have a particular preference. Certainly having the version defined in a single location is preferable, but this page lists several ways of doing this. They seem to advocate for @ragonneau's approach of reading from __init__.py. However, numpy for example defines the version in pyproject.toml (and their build system creates numpy/version.py with this information), so I think either option is fine.

Regarding what to do with the built packages, I agree that we don't need to do anything right now as pycutest installation now requires setuptools to be installed as a prerequisite. I guess further down the road it might be nice to improve this, but it doesn't seem that urgent (compared to the package build failing on 3.12).

jfowkes commented 8 months ago

Thanks @lindonroberts, I think you and @ragonneau have persuaded me that reading the version from __init__.py is cleaner as it doesn't require importlib and seems to be the number one preferred official approach. I've switched the PR over to this approach, could you please both review? It's important to get this change right before a bugfix release.

ragonneau commented 8 months ago

Hi @jfowkes,

I am not sure whether __version__ should go in __all__. Is it really necessary?

Cheers, Tom.

jfowkes commented 8 months ago

I am not sure whether __version__ should go in __all__. Is it really necessary?

@ragonneau looking more into this, it seems the purpose of __all__ is to define what is imported on a wildcard import of the form from pycutest import *: https://docs.python.org/3/tutorial/modules.html#importing-from-a-package I see no reason for __all__ to include __version__ so I have now removed it.