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

refactor(build): change setup.py to pyproject.toml and supress cython warning #293

Closed
wants to merge 7 commits into from

Conversation

hassec
Copy link
Contributor

@hassec hassec commented Feb 27, 2024

I was motivated to fix #289, but when starting to work on the old setup.py I was curious if you would be open to a bit of a modernisation, see below:

Changes in this PR:

  • Move package/build configuration into a pyproject.toml following the adopted python standards.
  • Only have a very minimal setup.py that translates omas_cython.pyx to omas_cython.c at the time the sdist package is created. This has the benefit that Cython is no longer a dependency for users. They *.c file still gets compiled during package install on the users computer, but that still is pretty quick and doesn't produce any warnings.

This PR also removes the need to manually traverse the file hierarchy to create packages and package_data lists. This is now automatically handled.

Editable installs still work by simply running:

pip install -e .

To build a sdist package using build run:

python -m build --sdist

from the root of the project. This creates an isolated virtual environment to build the sdist according to the spec in pyproject.toml.

@@ -864,23 +865,6 @@ def omas_global_quantities(imas_version=omas_rcparams['default_imas_version']):
return _global_quantities[imas_version]


# only attempt cython if effective user owns this copy of omas
# disabled for Windows: need to add check for file ownership under Windows
if os.name == 'nt' or os.geteuid() != os.stat(__file__).st_uid:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this change the compiled version of omas_cython should be available to everyone. Also windows users.

exclude = ["sphinx*"]

[tool.setuptools.dynamic]
version = {file = "omas/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.

This automatically sets the version to what is found in omas/version

@hassec hassec marked this pull request as draft February 27, 2024 01:45
@orso82
Copy link
Member

orso82 commented Feb 27, 2024

We're certainly very open!!! Thank you @hassec for taking a deep dive into the setup files of OMAS!!

Perhaps @kalling can comment here. I have not kept up to date with the latest and greatest ways to setup a Python package.

pyproject.toml Outdated
]
keywords = ["integrated modeling", "OMFIT", "IMAS", "ITER"]

requires-python = '>=3.8'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would require bumping this from 3.6 to 3.8.
3.6 was end of life in 2021, so I hope that this is ok?

Copy link
Member

Choose a reason for hiding this comment

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

OMFIT is at python=3.7 and uses omas as an integral part. What changes are you making here that require python>=3.8?

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 looked at what the recent versions of setuptools require.
But what would be needed here is support for PEP-621 which looks like it landed in setuptools version v61-0-0 which still supports >=3.7.

I could try and see giving if it works for 3.7.

Copy link
Contributor Author

@hassec hassec Feb 28, 2024

Choose a reason for hiding this comment

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

@smithsp I double checked and I was correct that in theory 3.7 should have been sufficient to enable usage of pyproject.toml based package configurations.

However, the problem here is the dependency on pygacode which is forced to version 0.71 from 2022 because that's the last version that supports 3.7, and that version does not correctly specify its build dependencies (requires numpy). This is fixed for versions >1.0 but those require python >=3.8 which is why the build here works for 3.8 but nor for 3.7.

Copy link
Member

Choose a reason for hiding this comment

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

We are actively working on migrating OMFIT to a later python version.

Copy link

Stale pull request message

@github-actions github-actions bot closed this May 6, 2024
@smithsp smithsp reopened this May 6, 2024
@github-actions github-actions bot closed this May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suppress spurious compiler warnings on first import of omas
3 participants