Transition to pyproject.toml#1147
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
0dd8b1e to
a3df50c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This reverts commit 9761a16.
PEP 621 does not allow `project.name` to be dynamic, and requires any build backend to fail if one declares it as such. Since we only need to change the name of the package when creating a nightly release, this adds a `change_name.py` script, which only runs in the CI, that can change the `project.name` entry in-place. Note that, since `tomllib` has only become a part of the standard Python library in version 3.11, we use the `tomli` (for reading) and `tomli-w` (for writing) packages for parsing the `pyproject.toml` file.
On MacOS there seems to be an issue with `scikit-build-core` when `build-dir` is unset, so now we explicitly set it when building the wheel.
|
I get the following version information for this branch: is that intended? |
We set |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Logfiles from GitLab pipeline #198340 (:white_check_mark:) have been uploaded here! Status and direct links: |
|
Let's summarize:
|
|
To answer your questions:
For the version issue, see #1199. We don't ship asan in the distributed wheels, so I don't think it's a big issue. Also, the failing test (ODE) is actually tested in the CI as part of the Pytest suite. EDIT: issue is here now for posterity #1200
Yes.
Ongoing discussion at BlueBrain/spack#2332, but I have no strong opinion whether we should still support Python 3.8 or not. |
BlueBrain/spack (as opposed to spack/spack) is by and large a bespoke version of Spack for BB5 (and maybe some EPFL controlled laptops). Therefore, there's a reasonably chance that avoiding the issue now means never encountering it. When someone does need Python 3.8, they'll first be sent to the archive of modulse and if that doesn't work one can put in the time to support 3.8. W.r.t the PR itself: nice work! Since the Spack PR does nothing for |
* replace `setup.py` with `pyproject.toml` * move all Python package requirements to single `requirements.txt` * remove checking Python package requirements in `CMakeLists.txt` * move Python bindings to own dir (python/nmodl), fixes #462 * add separate script for generating docs * add `packaging/change_name.py` script to as workaround to enable both NMODL and NMODL-nightly wheels in CI * update documentation and CI to reflect above changes --------- Co-authored-by: Luc Grosheintz <luc.grosheintz@gmail.com>
* replace `setup.py` with `pyproject.toml` * move all Python package requirements to single `requirements.txt` * remove checking Python package requirements in `CMakeLists.txt` * move Python bindings to own dir (python/nmodl), fixes BlueBrain/nmodl#462 * add separate script for generating docs * add `packaging/change_name.py` script to as workaround to enable both NMODL and NMODL-nightly wheels in CI * update documentation and CI to reflect above changes --------- Co-authored-by: Luc Grosheintz <luc.grosheintz@gmail.com> NMODL Repo SHA: BlueBrain/nmodl@3acc935
Overview and context of changes
This PR moves all of the build logic from the dynamic
setup.pyinto the new staticpyproject.tomlconfiguration file, as described in PEP 621. Since NMODL has a handy Python wrapper, it makes sense to use a modern Python build system.Some of the notable changes and explanations for them:
pip wheel . --no-deps, with an optional--wheel-dir [DIR]for outputscikit-build-core, and any options can be passed by adding-C [OPTION]=[VALUE]to the above call (on MacOS, this is needed to specify the build dir as it fails in a temp dir due to a bug in cmake)setup.pyand all various requirements files in favor ofpyproject.toml(except a singlerequirements.txtwhich has all of them too)setup.cfgoptions topyproject.tomlas well (notably flake8 and sphinx do not support it, so we leave those out for now)due to the fact thatusepipdoesn't provide a way of parsing thepyproject.tomlfile using the CLI, to install all dependencies (build, run, optional), we need to dopip install 'pip-tools>=7.4.0', followed bypip install -r <(pip-compile --all-build-deps --all-extras --no-strip-extras 2>&1)Note that this will also leave arequirements.txtin your current dir (not sure if this is a feature or a bug ofpip-compile)pip install -r requirements.txtto install requirementssetup.pyto a standalone scriptpackaging/generate_docs.shwhich generates the documentation (see docs for usage)pip install -e .(i.e. editable install) doesn't quite work due to the strange way we bundle everything, butpip install -C build-dir=[SOMEDIR]is fast enough so there isn't much of a difference. If we later merge Move Python bindings to own dir #1179 we can also sidestep any issues with importsNMODL_BUILD_WHEELto cmake config so we can signal we are building a wheel (this sets the appropriate flags automatically)includeandlibstuff from wheel because they're not needed (looking at you, fmtlib and eigen). Also fixes fmtlib places files outside of.datawhen not linking against Python #1153nmodl-nightlywheels sometimes), so as a workaround there is thepackaging/change_name.pyscript, which can be used for changing the name of the package (only relevant in the CI, so users shouldn't touch it)pyproject.tomldoesn't allow an empty version (we now call cmake after Python readspyproject.toml), so we can set it dynamically usingsetuptools_scm(uses git tags), or even statically usingSETUPTOOLS_SCM_PRETEND_VERSION(we seem to use something likegit describe --tags | cut -d '-' -f 1,2 | tr - .to set it) in the CI (I didn't want to do too much refactoring with versions)pywheeldir is gone, and the_binwrapperscript is moved tonmodldir with minimal changes so we can actually set the right script targetpython/nmodlinstead ofnmodl. Fixes Move nmodl python files into subdirectory #462.NMODL_WHEEL_VERSIONwas removed; if you want to specify a custom version of a wheel, useSETUPTOOLS_SCM_PRETEND_VERSIONinsteadStuff that works
pip wheel .(andpython -m build --wheel) actually builds an installable wheel. Note that the latter needs thebuildpackage to work properly (but this should only be ran in the CI anyway, most developers should usepip install --config-setting="build-dir=_build" -e .or similar for development and testing).Stuff that needs work
setuptools-scmseems to be able to do it without any file (see here), which is nice, but is Python-specific. Maybe this project? (EDIT: as a workaround, we can set the env variableSETUPTOOLS_SCM_PRETEND_VERSION_FOR_NMODL="$(git describe --tags | cut -d '-' -f 1,2 | tr - .)"to mimic the current behavior)build-system.requiresandproject.dependencieshave some duplication: figure out which ones are required when (build-time vs. run-time) (EDIT: separated the two into disjoint sets now, works as intended)build-system.requiresdependencies are also built inwheelhouse/when runningpip wheel(EDIT: it seemspip wheelcan be replaced bypython -m build --wheel, which doesn't have this issue EDIT 2: the--no-depsflag does exactly that so we don't needbuildat all)setup.py, figure out how to build it without burdeningsetup.pywith it.sphinxcomes with thesphinx-buildCLI utility, but is a bit cumbersome to use (EDIT: the docs are a bit difficult to refactor since they mix the version and NMODL installation. EDIT 2: preliminary docs seem to work)project.optional-dependencies.testandproject.optional-dependencies.docs)pyproject.tomlspec, explicitly mentions: "A build back-end MUST raise an error if the metadata specifies the name in dynamic.", so we can't use something that respects that PEP. A possible solution is to have a script in the CI which editspyproject.tomlto change the name of the project tonmodl-nightly(EDIT: as a PoC workaround, added achange_name.pyscript to thepackaging/subdirectory, that only runs in the CI, which can change the name of the project before we actually build it).pip install -e .works, andsetuptoolshas support for PEP 660) (EDIT 2:scikit-builddoes not support editable installs, butscikit-build-coredoes. Note that one should explicitly set--config-setting="build-dir=[PATH]"to re-use the build directory since build frontends delete them).TODO: maybe pin the earliest version ofwe don't need this if usingsetuptoolswhich has support for PEP 660?scikit-build-corebuild-system.build-diris not specified on MacOS, the build consistently fails (reproducible both in the CI and locally), with the complaint from bison that one of the files cannot be found. Not sure if this is a huge problem (we can always override this by specifying theSKBUILD_BUILD_DIRenv variable), but it's a bit annoying that it doesn't work OOTB (EDIT: this is possibly due to this bug in cmake itself; as a workaround, if specifying a temporary dir as a build dir, one can userealpath $(mktemp -d)instead of justmktemp -d)fmtlibrary-related stuff in theinstall_manifest.txt, which is impossible to ignore using eithersdist.excludeorwheel.exclude, so, right now,scikit-build-coreputs spuriousincludeandlib(orlib64on Linux) directories in the wheel which cannot be removed (ticket: fmtlib places files outside of.datawhen not linking against Python #1153) (EDIT: we can disable this usingset(FMT_INSTALL OFF)inCMakeLists.txt. EDIT 2: c.f. Fix standard default installation target presence fmtlib/fmt#3264 maybe there's a more clever way of doing this?)build-system.requiresdependencies using pip (since it vendors its own version of a TOML parser), but I am not very keen on being dependent on dubious enhancements to upstream. Of course, for Python 3.11+, it's possible to make a script which parsespyproject.tomlnatively that can then feed a list to either stdout or to a requirements.txt file (EDIT: this is now possible with a PoC script inpackaging/show_deps.py, which uses the vendoredtomlipackage in pip itself, or the nativetomllibif running on Python 3.11. Note that there's also pip-tools, but I haven't found a way for it to output thebuild-system.requiresdependencies. EDIT 2: once this commit gets into a release, then we should be able to use--all-build-depsto get those as well. EDIT 3:pip-tools7.4.0 and above can now usepip-compile --all-build-depsto get the build-time dependencies. This can also be used with the--all-extrasto get all dependencies installed)pip-tools, one must runpip install 'pip-tools>=7.4.0'followed bypip install -r <(pip-compile --all-build-deps --all-extras --no-strip-extras 2>&1)