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 111 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
bash 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
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@shimwell
Copy link
Member Author

shimwell commented Dec 2, 2024

The coverage command is in the CI here and it has flags for directories to include and ignore.
https://github.com/shimwell/openmc/blob/b2abffb09915e9b7484e14050ecfad079061413f/.github/workflows/ci.yml#L160C1-L161C58
I'm wondering if we have an issue as the cpp and python are now both in the same directory.

@shimwell
Copy link
Member Author

shimwell commented Dec 3, 2024

@ahnaf-tahmid-chowdhury congratulations for getting the coverage to pass as well. You are a super star.

I think this is ready for a review if any one has time

@shimwell shimwell force-pushed the scikit_build_core_wheel branch from 824d1f4 to 3ede515 Compare December 12, 2024 13:14
openmc/lib/__init__.py Outdated Show resolved Hide resolved
Comment on lines 64 to 70
warnings.warn(
"It seems OpenMC is being run from its source directory. "
"This setup is not recommended as it may lead to unexpected behavior, "
"such as conflicts between source and installed versions. "
"Please run your script from outside the OpenMC source tree.",
RuntimeWarning
)
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've been running outside of the source directory and getting this warning pop up

Copy link
Contributor

Choose a reason for hiding this comment

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

checking

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps we just remove this warning, currently in the develop branch we don't print a warning for running inside the source directory.

Comment on lines +58 to +64
warnings.warn(
"It seems OpenMC is being run from its source directory. "
"This setup is not recommended as it may lead to unexpected behavior, "
"such as conflicts between source and installed versions. "
"Please run your script from outside the OpenMC source tree.",
RuntimeWarning
)
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 think it is perhaps better to removing this warning as it was not there before the PR

Suggested change
warnings.warn(
"It seems OpenMC is being run from its source directory. "
"This setup is not recommended as it may lead to unexpected behavior, "
"such as conflicts between source and installed versions. "
"Please run your script from outside the OpenMC source tree.",
RuntimeWarning
)

tools/ci/gha-script.sh Outdated Show resolved Hide resolved
@jon-proximafusion
Copy link
Contributor

Just solved a couple of conflicts to keep this branch up to date

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