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

[WIP] Build pyproj toml #948

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

asoplata
Copy link
Collaborator

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

Removed metadata that is now contained in the pyproject.toml. setup.py  now only serves to house the custom build steps related to building Neuron's mod files.
@asoplata
Copy link
Collaborator Author

asoplata commented Nov 21, 2024

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

@gtdang
Copy link
Collaborator

gtdang commented Nov 21, 2024

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.

include-package-data = true

[tool.setuptools.dynamic]
version = {attr = "hnn_core.__version__"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

will this actually work? I suspect it will break ...

Copy link
Collaborator

@gtdang gtdang Nov 22, 2024

Choose a reason for hiding this comment

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

Here's the documentation on specifying dynamic metadata. I think it should work, it's pretty commonplace with toml specification. Just need the version defined in the top-level __init__.py in the package.

Copy link
Collaborator

Choose a reason for hiding this comment

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

try ... I think hnn_core.__version__ is not defined before installation. Maybe it works !

@jasmainak
Copy link
Collaborator

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

@asoplata asoplata changed the title Build pyproj toml [WIP] Build pyproj toml Nov 22, 2024
@asoplata
Copy link
Collaborator Author

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.

@gtdang
Copy link
Collaborator

gtdang commented Nov 22, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants