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

Use pyproject.toml to configure the build system #74

Merged
merged 4 commits into from
Jan 22, 2025

Conversation

rafmudaf
Copy link
Collaborator

@rafmudaf rafmudaf commented Dec 5, 2024

Transition to pyproject.toml

This pull request transitions windIO from using the setup.py / Setuptools configuration to pyproject.toml / [build-system] table which includes Setuptools by default. It was suggested in #46. My understanding is that this mostly impacts the ability to use additional build systems like Poetry.

While I don't think this will practically change anything for most of us, it has become the standard in the Python community. There's some information on this transition from the Python organization.

Some things to discuss

The first commit directly converts the existing setup.py to the new pyproject.toml. In testing, I noticed that the build configuration states support for Python >= 3.7, so I expanded the CI to include all supported versions in the test matrix (3.7 to 3.13, the latest Python) in the second commit. This leads to failing installations (see here) for Python 3.7 on Ubuntu and Python 3.7 and 3.8 on Windows due to a NetCDF dependency (NetCDF4 requires at least Python 3.8). So this might require raising the minimum Python version to 3.8 in windIO.

NetCDF is required because xarray is a dependency of windIO. xarray requires Python >= 3.10, so maybe windIO should follow that and require Python = 3.10.

Alternatively, windIO could pin the versions of these dependencies to versions where they supported older versions of Python.

However, since windIO is a fairly simple project, it's worth considering whether these dependencies are necessary to begin with. Both NumPy and xarray are required because there a PyWake interface is included, so data structures specific to PyWake have to be created in windIO. It's outside the scope of this pull request, but given the impact on these changes I think it's worth discussing further where code-specific interfaces to windIO should be hosted. This has been partially discussed in #12 and #23.

@rafmudaf
Copy link
Collaborator Author

rafmudaf commented Dec 5, 2024

cc @kenloen

@kenloen
Copy link
Collaborator

kenloen commented Dec 6, 2024

Great work @rafmudaf.

I do not recall that I had seen your discussion post #23, but I fully agree with the structure that you propose there.
I would like to see a minimum dependency for the main windIO repo to make it a light dependency for any project. As shown with this pull request, having dependencies as xarray and subsequent netcfd is a risk and I would really like that part to be an optional dependency.

The issue is that the plant schema relies on a custom constructor to load data into Python. That constructor uses PyYAML syntax to include other files in the same Python data object. I would suggest that we opt not to use YAML syntax for either schema (meaning not using anchors) or the !include which seems to be a PyYAML construction (I can at least not find it here). Instead, I would suggest using the $ref conversion from json-schema.

There is various implementations of the $ref syntax for both YAML and JSON:

But it might be just as easy (as well as not relying on unmaintained repos) to add our own constructer for YAML, which can read files using the $ref conversion.
What do you think about that @kilojoules, @bayc, @WindfarmDesigner ?

@rafmudaf rafmudaf added python-library Related to the python library overall-repo Related to the overall organization of the repo labels Jan 14, 2025
@rafmudaf rafmudaf self-assigned this Jan 14, 2025
@rafmudaf rafmudaf changed the base branch from develop to main January 14, 2025 19:33
@rafmudaf rafmudaf mentioned this pull request Jan 14, 2025
@rafmudaf rafmudaf changed the base branch from main to develop January 14, 2025 19:45
@rafmudaf
Copy link
Collaborator Author

To deal with failing tests, I've bumped the minimum Python version to 3.8. There's still a failing test for Windows due to the NetCDF dependency. We could also consider to go to 3.9. See the Status of Python Versions.

@rafmudaf rafmudaf deleted the branch IEAWindSystems:main January 16, 2025 16:49
@rafmudaf rafmudaf closed this Jan 16, 2025
@rafmudaf rafmudaf reopened this Jan 16, 2025
@rafmudaf rafmudaf changed the base branch from develop to main January 16, 2025 16:50
@kilojoules
Copy link
Collaborator

It seems fine to only test 3.9 and above

@ptrbortolotti ptrbortolotti requested a review from kenloen January 22, 2025 15:28
Decision made in a working meeting, since 3.8 is no longer supported windIO will not officially support it.
kenloen

This comment was marked as outdated.

@rafmudaf
Copy link
Collaborator Author

@ptrbortolotti @fzahle If there are no changes from your maintainer review, this is ready to roll

@ptrbortolotti ptrbortolotti merged commit b256963 into IEAWindSystems:main Jan 22, 2025
10 checks passed
@rafmudaf rafmudaf deleted the pyproject branch January 22, 2025 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
overall-repo Related to the overall organization of the repo python-library Related to the python library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants