hnn-core icon indicating copy to clipboard operation
hnn-core copied to clipboard

[WIP] Build pyproj toml

Open asoplata opened this issue 1 year ago • 5 comments

Making a PR for @gtdang 's work transitioning to pyproject.toml

asoplata avatar Nov 21 '24 20:11 asoplata

We should also use this as an opportunity to consolidate other top-level configuration files into pyproject.toml:

  • [ ] pytest.ini: seems we can https://docs.pytest.org/en/7.1.x/reference/customize.html#pyproject-toml
  • [ ] .coveragerc: seems we can https://coverage.readthedocs.io/en/latest/config.html#toml-syntax
  • [ ] .mailmap ???
  • [ ] MANIFEST.in ???
  • [ ] setup.cfg seems maybe not https://flake8.pycqa.org/en/latest/user/configuration.html#configuration-locations , but if we change to ruff linting (see #934) then we can delete this file

asoplata avatar Nov 21 '24 22:11 asoplata

fyi. I thought I could get away with adding mpi4py to the parallel optional dependencies because you can specify os conditions in the pyproject.toml. Like:

[project.optional-dependencies]
opt = ['scikit-learn']
...
parallel = ['joblib', 'psutil', 'mpi4py ; platform_system != "Windows']

It actually worked preventing it from installing on Windows however... it broke the macOS unit test workflows because it was mixing and matching: getting mpi4py from pip and OpenMPI from conda. It couldn't find OpenMPI in the path for some reason.

gtdang avatar Nov 21 '24 22:11 gtdang

good idea to consolidate configuration in pyproject

I suggest making the changes iteratively in small PRs. For now, if it works with pytest.ini and coveragerc I would be a happy man

jasmainak avatar Nov 22 '24 19:11 jasmainak

Regarding mpi4py, unfortunately it needs to have its install handled separately from the official Python dependencies of our package. We should definitely not include it in our pyproject.toml, and I have not yet updated the official installation documentation with my install investigation into OpenMPI install.

asoplata avatar Nov 22 '24 19:11 asoplata

The parallel dependency group is a little weird... joblib is the default if you don't have MPI. The only reason you wouldn't want it installed is if you have MPI set up already... And then psutil is only used with MPI. If mpi4py must be externally installed I guess we should just pair psutil with that installation.

It might be best to get rid of that dependency group because it's super confusing.

gtdang avatar Nov 22 '24 19:11 gtdang