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

wheel building with scikit build core #3087

Open
wants to merge 90 commits into
base: develop
Choose a base branch
from

Conversation

shimwell
Copy link
Member

@shimwell shimwell commented Jul 19, 2024

Description

Just making this PR to show a bit of progress with the production of wheels using scikit build core. Tagging @hassec as he made the OpenMCExtension part of this PR.

Running pip install . in the root folder of this repo and this will compile openmc and build a wheel and then install the python package and openmc executable from the wheel with default cmake args.

It is also possible to pass in cmake args via the command line in the same line as the pip install command.

you can also run pip install . --no-clean This will do the same but it won't delete the wheel afterwards. This allows you to inspect the wheel and manually install from the wheel. The path to the wheel is printed to the terminal and will be in your /tmp folder

I've added some if(SKBUILD) statements to the CMakeLists.txt to help insure the new commands only run if the process is being driven by Scikit build core and therefore this preserves the current behavior when someone wants to build the executable with cmake

I can follow up this PR to include ciwheelbuilder to make wheels for different OS versions

Fixes # (issue)

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 15) on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@shimwell
Copy link
Member Author

shimwell commented Jul 19, 2024

for anyone interested in wheels that contain DAGMC, Double Down, Embree and Moab options in the OpenMC executable then

I have that working on this branch, clone or checkout that branch
https://github.com/shimwell/openmc/tree/adding_dagmc_to_scikit_build_core
then run the script
pre_wheel_install_dagmc_embree_dd_moab.sh
then run
pip install . --no-clean
and your wheel will be in a sub directory of the /tmp folder (full path is printed to terminal)

@shimwell
Copy link
Member Author

shimwell commented Jul 23, 2024

just to note a new pip version is needed to pass in cmake arguments via the command line, if you get this error then upgrade pip (I'm using version 24.1.2)

pip install . --config-settings=cmake.args=-DCMAKE_BUILD_TYPE=RELEASE
>>> no such option: --config-settings

solve with

python -m pip install --upgrade pip

@shimwell
Copy link
Member Author

shimwell commented Jul 26, 2024

nice the docs on readthedocs are now working, it needed a bit of tweaking as they now installing the package with this scikit build core method.

tools/ci/gha-install.py Outdated Show resolved Hide resolved
tools/ci/gha-install.py Outdated Show resolved Hide resolved
@@ -1,2 +1,2 @@
k-combined:
1.716873E+00 5.266107E-02
1.716873E+00 5.266094E-02
Copy link
Contributor

Choose a reason for hiding this comment

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

@shimwell, is this one added unintentionally?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was seeing small changes to the regression test values in the CI so I changed this value to see if that helped. Perhaps this is no longer necessary. Ideally this change would not be need and we would keep the values the same as the original.

@ahnaf-tahmid-chowdhury
Copy link
Contributor

Only one test is failing, stating that data/compton_profiles.h5 is not found. I tested this on my machine, and it was working. I'm not sure why sdist is not including it.

@shimwell
Copy link
Member Author

shimwell commented Nov 8, 2024

I've solved the merge conflicts and kept this upto date with develop
I've also reduced the diff in a few places so this PR is slightly more minimal

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

Successfully merging this pull request may close these issues.

3 participants