[ENH] Move to pyproject.toml#608
Conversation
9efeb6d to
e49716a
Compare
…nd optional dependencies, this avoids need of dedicated CI to check and maintain requirements files
1115334 to
07d1295
Compare
07d1295 to
d74fa8d
Compare
…or system packages
Added a finish job to combine coveralls report; Added pytest-cov pluggin to req-test to avoid duplicated install statements in CI file;
…w due to Node js issue
|
Postponing addition of GPU tests with group runner available on jugit.fz-juelich.de to a different PR. |
|
Once PR approved, full CI should be triggered. (Run once for each PR approval) |
mdenker
left a comment
There was a problem hiding this comment.
Thank you for this extensive PR, besides the comment about Python versions, it looks good to me so far -- I guess we need to see how the remaining CI jobs work out. I tested installs on Linux and Windows, which seem to work (including the fim module unter linux). However, the Python 3.8 issue that prevents installation of the package in a uv managed pip environment is something we should address.
| "Topic :: Scientific/Engineering"] | ||
| keywords = ["neuroscience", "neurophysiology", "electrophysiology", "statistics", "data-analysis"] | ||
| readme = {file = "README.md", content-type = "text/markdown"} | ||
| requires-python = ">=3.8" |
There was a problem hiding this comment.
I feel like this line is giving me problems when doing the following:
- Create a new environment using
uv venv - Doing a
uv pip install .in the elephant directory - Calling
uv run pythonm which gives me an error about dependencies not being satisfyable:uv responds that while I am currently using Python 3.14, thereuires-pythonindicates >=3.8. This is however incompatible with numpy 2. Therefore, uv fails, as it cannot resolve for any of the support python versions. I do not fully understand this logic, however, I guess we should drop 3.8 despite this being a hot fix. Or maybe I am missing something?
There was a problem hiding this comment.
Yup I could recreate this issue, it's an error from my side. The setup.py on our latest release has a requirement on python>=3.9, so I'll fix this now on the toml file.
As a side note, the latest release of numpy (2.4.6) has a requirement of python>=3.11, though lower version of numpy 2.0 still support python 3.9. So we should definitely fix this on our next minor release.
…request_review trigger; Made all other jobs dependent on setup job, so they are also skipped based on setup job
|
Test duplication in case of pull_request_review trigger (i.e. both pull_request_review triggered tests pollutes the existing PR checks summary made with the checks triggered on PR synchronize) is the right behavior on GitHub and cannot be avoided as GitHub treats these two as separate events. Refer https://github.com/orgs/community/discussions/26940 As some form of relief, an if clause has been added on the setup job that makes it only run when pull_request_review is approved and not when a review comment is added. |
kohlerca
left a comment
There was a problem hiding this comment.
The PR looks good to me. I tested locally in Ubuntu, using mamba and uv, and the installation succeeds.
|
This is a long one!
Original PR Body by Moritz
This PR moves Elephant from setup.py to pyproject.toml, aligning with PEP 518.
Key changes include:
pyproject.tomlsetup.pyis still needed to compile the C-code for the fim module.The reason is that we need OS dependend compile args which are dynamically defined in
setup.py.The
pyproject.tomldoes not natively support conditional logic like os_name == 'windows' directly within the configuration. The TOML format is static and doesn't allow for dynamic evaluation of conditions based on the operating system.To achieve platform-specific compile arguments, we have to use a Python script, such as a
setup.py, which can programmatically determine the OS and apply the correct compile arguments.In this context, as a configuration file for setuptools,
setup.pyis not deprecated, see also: https://packaging.python.org/en/latest/discussions/setup-py-deprecated/Changes
pyproject.toml
pyproject.tomlwith full project metadata (name, version, description, authors, license,URLs, classifiers)
License ::classifier stringsetuptools.find_packages(), this correctly picks upsubpackages such as
elephant.assetinclude-package-data = trueso non-Python data files are included in the wheel installsixdependencyviziphantto thetutorialsoptional-dependency grouprequirements/*.txtfiles. This keeps a single source of truth for dependenciesCI modernisation (
.github/workflows/CI.yml)matrix.include, eliminating duplication.github/actions/restore-elephant-data) to avoid code duplication of restore action that restored cached elephant-dataset and is shared by the pip, MPI and conda jobsenableCrossOsArchive: trueon the data cache so a cache built on one OS can be restored onanother, this was added to build to both restore action and cache build action (
.github/workflows/cache_elephant_data.yml)actions/cache/,actions/setup-pythonandactions/checkoutto newer versions to fix issue with Node.js 20 actions being deprecatedfinishjob combines them at the end. This is to fix the earlier race condition where last coverall job was being usede to add PR comment.pytest-covtorequirements/requirements-tests.txtto avoid duplicate install steps for each jobfastworkflow-dispatch boolean input that limits the matrix to a subset of jobsEnvironment files
requirements/environment*.ymlconda environment files to import from the correspondingrequirements/*.txtfiles instead of duplicating the dependency list (system-level conda-only packages are still listed directly)Bug fix for Windows path
elephant/datasets.py: switched tolocal_file.as_posix()when constructing theValueErrormessage indownload_datasets. On Windows,pathlibrendered backslashes, causingtest_download_with_cache_dir_with_failed_integrity_checkto fail because the assertion looks for the POSIX-stylerepo_pathsubstring.