JDBetteridge/update build system #61
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,41 +1,58 @@ | ||
| [project.optional-dependencies] | ||
| dev = [ | ||
| "pytest", | ||
| "pylint", | ||
| ] | ||
| ngsolve = [ | ||
| "ngsolve", | ||
| ] | ||
| firedrake = [ | ||
| "firedrake", | ||
| ] | ||
|
|
||
| [tool.poetry] | ||
| name = "ngsPETSc" | ||
| version = "0.0.6" | ||
| description = "NGSolve/Netgen interface to PETSc" | ||
| authors = [ | ||
| "Umberto Zerbinati <[email protected]>", | ||
| "Patrick E. Farrell <[email protected]>", | ||
| "Stefano Zampini <[email protected]>", | ||
| "Jack Betteridge <[email protected]>", | ||
| ] | ||
| maintainers = [ | ||
| "Umberto Zerbinati <[email protected]>", | ||
| ] | ||
| readme = "README.md" | ||
| licence = "MIT" | ||
| packages = [ | ||
| {include = "ngsPETSc"} | ||
| ] | ||
| documentation = "https://ngspetsc.readthedocs.io/en/latest/" | ||
| repository = "https://github.com/NGSolve/ngsPETSc" | ||
| classifiers = [ | ||
| "Programming Language :: Python :: 3", | ||
| "License :: OSI Approved :: MIT License", | ||
| "Operating System :: OS Independent", | ||
| "Development Status :: 3 - Alpha", | ||
| ] | ||
|
|
||
| [tool.poetry.dependencies] | ||
| python = "^3.9" | ||
| netgen-mesher = "^6.2" | ||
| netgen-occt = "^7.8" | ||
| petsc4py = "^3.22.1" | ||
| mpi4py = "^4" | ||
| numpy = "^2.0" | ||
| scipy = "^1" | ||
|
There was a problem hiding this comment. Are these explicit dependencies only needed to build the wheel or they are also used to install the package via pip? If the latter case, then they seem too strict There was a problem hiding this comment. They probably are a bit strict, but at least initially I want to get something working. The main aim of this PR (from Firedrake's viewpoint) is to use versioned ngsPETSc for the CI. Of course it's up to ngsPETSc which versions of dependencies it wasn't to support! There was a problem hiding this comment. I don't use firedrake with ngsPETSc, and we should find a way to relax those requirements here. If you need these specific requirements for firedrake CI you can do it there, not here, unless I'm missing something. There was a problem hiding this comment. I think we're agreeing with each other. Poetry by default adds a very constrained version range, these values (except for petsc4py, which is more strict and scipy which is less strict) have been edited to be pinned to minor releases, we can probably relax to major releases for most packages. I don't know enough about netgen-mesher and netgen-occt to comment further on those constraints though. There was a problem hiding this comment. Can we get rid of the scipy dependency There was a problem hiding this comment. Sure, although it's probably it's own PR. I introduced it as optional but was told it was fine to have a hard dependency so removed the alternative. See: #48 (comment) There was a problem hiding this comment. We didn't have this dependence a while ago, after @JDBetteridge refactored ngsPETSc curve mesh support for Firedrake he opted to use There was a problem hiding this comment. I'm not sure having scipy as a dependency is the end of the world, it's an easy to install well supported project and comes by default on some Python distributions. |
||
|
|
||
| [tool.poetry.group.dev.dependencies] | ||
| pytest = "^8.3" | ||
| pylint = "^3.3" | ||
|
|
||
| [tool.poetry.group.ngsolve.dependencies] | ||
|
There was a problem hiding this comment. Why this is optional? ngsPETSc is a package that offers PETSc solvers to ngsolve, it shouldn't be optional There was a problem hiding this comment. ngsPETSc at its bare minimum, only provides netgen meshes to PETSc and vice-versa, which only requires netgen for this reason I think ngsolve can be interpreted as an optional dependency while netgen is not optional. There was a problem hiding this comment. This also works around the issue that netgen packages have a completely different name when installed from source and the explanation I got after questioning this code: There was a problem hiding this comment. I thought the business of ngsPETSc is to provide parallel solver support for ngsolve, not meshes, see also https://docu.ngsolve.org/latest/i-tutorials/index.html#mpi-parallelization-and-cuda-support. |
||
| ngsolve = "^6.2" | ||
|
|
||
| [build-system] | ||
| requires = ["poetry-core"] | ||
| build-backend = "poetry.core.masonry.api" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a look at ngsPETSc code, it seems there's no need to have
mpi4pylisted here, if not for a few occurrences of MPI.COMM_WORLD that can be replaced byPETSc.COMM_WORLD.tompi4py(). @UZerbinati How mpi4py is handled by the netgen related packages?I just tried to install with this branch and got this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not good ! I'll replace the use of
MPI.COMM_WORLDas you suggested.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how mpi4py is handled by the netgen related packages? @mhochsteger might know more since I remember he put a lot of work into making ngsolve with mpi support pip installable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it could be an issue with your MPI installation?
4.0 is using large count datatypes, which causes some headaches with existing MPI installations that don't conform to the MPI 4.0 standard.
It'd be nice to just suck down the mpi4py wheel, but that isn't available on PyPI. (...yet?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why users wouldn't want mpi4py TBH!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue with mpi4py 3.1.6, not a mpi4 problem. Does it work for you? I have mpich on fedora and installed ngspetsc using
pip install -e .letting the build figure out the dependenciesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can make the same argument with many other useful python packages. The point is that if a package does not explicitly use another one, it should not list it as a dependency. The only reason it is a dependency now is because of the misuse of
MPI.COMM_WORLDThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NGSolve/netgen#196
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a fresh python virtual environment
pip install ngsPETSc ngsolveallows me to run all tests.I'm working on Archlinux and OpenMPI, I'll try next on a fresh install with MPICH!