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

New build system based on scikit-build-core #383

Merged
merged 28 commits into from
Nov 27, 2023
Merged

New build system based on scikit-build-core #383

merged 28 commits into from
Nov 27, 2023

Conversation

mbkumar
Copy link
Collaborator

@mbkumar mbkumar commented Nov 20, 2023

I replaced the deprecated setup.py based build framework with a new build system based on scikit-build-core, which in turn uses hatchling as its backend. All the project meta-data is now in pyproject.toml. setup.py and setup.cfg are removed. Pyproject.toml can also contain options for various tools used in the project. At present, ruff configuration is written to it. In the future, I'll add more configurations to it, and remove the configuration files scattered around the project.

Copy link

codecov bot commented Nov 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (295f41e) 91.40% compared to head (aa8a6c8) 91.40%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #383   +/-   ##
=======================================
  Coverage   91.40%   91.40%           
=======================================
  Files          72       72           
  Lines       12659    12659           
=======================================
  Hits        11571    11571           
  Misses       1088     1088           
Flag Coverage Δ
unittests 91.40% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mbkumar
Copy link
Collaborator Author

mbkumar commented Nov 21, 2023

I think this is ready for review.

@mbkumar
Copy link
Collaborator Author

mbkumar commented Nov 21, 2023

The tests are passing, but the coverage data collection is failing for some reason. I am not seeing the behavior exhibited by the runner on my Mac laptop.

@@ -88,7 +88,7 @@ jobs:
- name: Install python dependencies
run: |
sudo apt-get install graphviz graphviz-dev
pip install wheel numpy scipy f90nml h5py scikit-build cmake qsc sympy pyevtk matplotlib ninja plotly networkx pygraphviz
pip install wheel numpy scipy f90nml h5py scikit-build cmake qsc sympy pyevtk matplotlib ninja plotly networkx pygraphviz mpi4py py_spec pyoculus h5py
Copy link
Contributor

Choose a reason for hiding this comment

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

h5py is listed twice here.

I'm trying to remember why all these packages are manually pip installed here. Wouldn't they automatically be installed when simsopt is pip installed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it was the holdover from the initial days, when we were building wheels first and then installing from the wheels. Still that does not explain the reason. Let me clean it up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I checked it again. We are installing them because SPEC build requires them. SPEC installation setup is stagnant. I implemented a minimal setup when building the Python wrappers. No one updated it afterward.

run: pip install -v .[MPI]
run: |
pip install -v .
pip install mpi4py
Copy link
Contributor

Choose a reason for hiding this comment

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

What wasn't working about the original line here? Doesn't [MPI] work for installing mpi4py?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For some reason, mpi4py was not getting installed. The runner is acting very peculiar and I am not sure why.

run: pip install -v .[MPI,SPEC]
run: |
pip install -v .
pip install mpi4py py_spec pyoculus h5py
Copy link
Contributor

Choose a reason for hiding this comment

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

What wasn't working about the original line here? Doesn't [MPI,SPEC] work for installing mpi4py py_spec pyoculus h5py?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. Not sure why.

@mbkumar
Copy link
Collaborator Author

mbkumar commented Nov 26, 2023

Fixed the coverage combine issue. Still not sure why it worked before and not now.

@mbkumar
Copy link
Collaborator Author

mbkumar commented Nov 27, 2023

@landreman This is ready for final review.

@mbkumar mbkumar merged commit d0aa01a into master Nov 27, 2023
47 checks passed
@mbkumar mbkumar deleted the scikit branch November 27, 2023 14:57
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.

2 participants