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

Migrate to pyproject.toml #653

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

Sand-jrd
Copy link
Contributor

@Sand-jrd Sand-jrd commented Dec 5, 2024

  • Updated pyproject.toml with the same project description, classifiers, and authors as in setup.py.
  • Removed setup.py from the root directory to avoid conflicts with pyproject.toml.
  • The package version is dynamically determined using Git tags.
  • Optional dependencies are explicitly defined in the [project.optional-dependencies] section. (pyproject.toml does not support directly reading dependencies from a .txt file)
  • Removed requirements-dev.txt as it is no longer needed.
  • Updated README to include instructions for installing optional dependencies using pip install .[dev].

- Updated `pyproject.toml` with the same project description, classifiers, and authors as in `setup.py`.
- Removed `setup.py` from the root directory to avoid conflicts with `pyproject.toml`.
- The package version is dynamically determined using Git tags.
- Optional dependencies are explicitly defined in the `[project.optional-dependencies]` section. (pyproject.toml does not support directly reading dependencies from a .txt file)
- Removed `requirements-dev.txt` as it is no longer needed.
- Updated `README` to include instructions for installing optional dependencies using `pip install .[dev]`.
Migrate project metadata to pyproject.toml

- Updated `pyproject.toml` with the same project description, classifiers, and authors as in `setup.py`.
- Removed `setup.py` from the root directory to avoid conflicts with `pyproject.toml`.
- The package version is now dynamically determined using Git tags.
- Optional dependencies are explicitly defined in the `[project.optional-dependencies]` section.
- Removed `requirements-dev.txt` as it is no longer needed.
- Updated `README` to include instructions for installing optional dependencies using `pip install .[dev]`.
@Sand-jrd
Copy link
Contributor Author

Sand-jrd commented Dec 5, 2024

Either re-create requirements-dev.txt or update pip install -r requirements-dev.txt to pip install .[dev]
in workflows ci.yml to fix failed checks

Keep the requirements-dev.txt file so it is possible to run pip install -r requirements-dev.txt and install dependencies alone
@Sand-jrd
Copy link
Contributor Author

Sand-jrd commented Dec 5, 2024

Either re-create requirements-dev.txt or update pip install -r requirements-dev.txt to pip install .[dev] in workflows ci.yml to fix failed checks

After thinking about it, I believe the best option is to keep the requirements-dev.txt file so that it is possible to run pip install -r requirements-dev.txt and install the dependencies individually.

@VChristiaens
Copy link
Contributor

Thanks a lot @Sand-jrd.
So far we have been using setup.py file to package VIP before publishing each new version release to pypi.
I guess an equivalent command to create a sdist and a bdist-wheel could be: python -m build --sdist --wheel, right ?
Would you be able to adapt the "Makefile" file accordingly, in particular for the pypi and pypi-test commands, and include it to this pull request?
I think that's the only important place where setup.py is also used, but feel free to double-check.

@VChristiaens
Copy link
Contributor

And regarding requirements-dev.txt, I think it is indeed safer to keep it. It is currently used when building an environment to run the test suite which is defined in the tests folder and triggered at each new push to VIP (see .github/workflows/ci.yml).

@Sand-jrd
Copy link
Contributor Author

Sand-jrd commented Jan 7, 2025

Thanks a lot @Sand-jrd. So far we have been using setup.py file to package VIP before publishing each new version release to pypi. I guess an equivalent command to create a sdist and a bdist-wheel could be: python -m build --sdist --wheel, right ? Would you be able to adapt the "Makefile" file accordingly, in particular for the pypi and pypi-test commands, and include it to this pull request? I think that's the only important place where setup.py is also used, but feel free to double-check.

Okay, I’ll do that. Yes, python -m build --sdist --wheel is also what I found online. I’ve replaced the two command lines in the Makefile accordingly. There is no other things to change in the makefile I think.

pyproject.toml Outdated
'Operating System :: MacOS :: MacOS X',
'Operating System :: POSIX :: Linux',
'Natural Language :: English',
'Programming Language :: Python :: 3.6',
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't these versions be compatible with the python version requirement above (and the versions also listed in the readme)?

'Intended Audience :: Science/Research',
'License :: OSI Approved :: MIT License',
'Operating System :: MacOS :: MacOS X',
'Operating System :: POSIX :: Linux',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also be compatible with Windows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Um I blindly copied the description classifiers from the previous setup; I guess it’s a good time to update them indeed

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’ve just updated them—both the Windows classifier and the Python version, as you pointed out.

pyproject.toml Outdated

[tool.setuptools_scm]
version_scheme = "guess-next-dev"
local_scheme = "no-local-version"
Copy link
Contributor

@VChristiaens VChristiaens Jan 7, 2025

Choose a reason for hiding this comment

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

If this means no more manual definition of the version in a file is needed, could you also remove this line?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth adding this line to have it automatically written somewhere:
version_file = "vip_hci/_version.py"

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, "guess-next-dev" uses previously released tags from github so there is no need for version_file = "vip_hci/_version.py". If you don’t like this to be automatic and prefer to use version_file = "vip_hci/_version.py", we can also do that, but in that case, we need to leave the version line in the __init__.py file.

So it's one of the two options, not both. I don't understand which option you prefer ?

I really believe automatic versioning is better, but that’s up to you. In this case, I’ll remove the line from the init.py file that I forgot to delete.

Copy link
Contributor

Choose a reason for hiding this comment

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

My preference is also for automatic versioning, but from quickly skimming the pypi page of setuptools-scm I had the impression one could also have the version (automatically) written in a file with the version_file line. But if I misunderstood, and this means instead that it's read from a manually edited file, then the version_file line can be removed indeed.

Copy link
Contributor Author

@Sand-jrd Sand-jrd Jan 8, 2025

Choose a reason for hiding this comment

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

Ah, okay. I thought you wanted the version to be written (by hand) somewhere and then read. I've just made the two-lines changes you asked for.

@Sand-jrd
Copy link
Contributor Author

Sand-jrd commented Jan 8, 2025

I followed the instructions from ‘version at runtime - Section Python package metadata - version at runtime’. However, with this fix, there might not always be a version argument, and I couldn't find a clean workaround for this. Let me know your thoughts

@Sand-jrd
Copy link
Contributor Author

Sand-jrd commented Jan 9, 2025

The workaround (the strongly discouraged option from the user guide)

from setuptools_scm import get_version
__version__ = get_version()

or

__version__ = importlib.metadata.version(__name__)

I see three possibilities :

1 - We keep automatic versioning and the version argument, but that means we have to add the library to the requirements.
2 - Remove the version argument entirely, but that will be a problem for object Saveable(object) -> save(self, filename) //Save a VIP object to a npz file. in config.until_conf.
3 - Keep the old, non-automatic versioning approach.

There is a thread about this topic

@VChristiaens
Copy link
Contributor

I think it'd be good to keep a __version__ attribute for the package, that can easily be checked upon importing VIP. So I'd opt for solution 1.

, but that means we have to add the library to the requirements.

I guess you mean the setuptools_scm solution, right?
Could this line __version__ = importlib.metadata.version(__name__) be used in conf to solve the current bug?

@Sand-jrd
Copy link
Contributor Author

Sand-jrd commented Jan 21, 2025

So you mean solution 1? Adding__version__ = importlib.metadata.version(__name__)in __init__.py? This line in conf could work too, if the goal is to ensure there's always a __version__ attribute available, then defining it in __init__.py makes more sens.

I've committed the solution.

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.

2 participants