Skip to content

Scikit build core wheel minimal#67

Open
jon-proximafusion wants to merge 164 commits intodevelopfrom
scikit_build_core_wheel_minimal
Open

Scikit build core wheel minimal#67
jon-proximafusion wants to merge 164 commits intodevelopfrom
scikit_build_core_wheel_minimal

Conversation

@jon-proximafusion
Copy link
Collaborator

@jon-proximafusion jon-proximafusion commented Feb 11, 2025

test PR trying to reduce this openmc PR to a more minimal PR that only does the scikit build core changes

FYI @ahnaf-tahmid-chowdhury

@ahnaf-tahmid-chowdhury
Copy link
Collaborator

Tests are failing due to an AssertionError. I believe all necessary files have been copied to this branch correctly, but I noticed some changes that need to be reviewed. Please check which files should be removed.

Regarding OpenMC's integration with NCrystal, building OpenMC with NCrystal creates a dependency on libNCrystal.so. While this currently works, it's not an efficient approach since we have to load it through subprocess. One possible fix is setting LD_LIBRARY_PATH, but that introduces additional complexity. Moreover, this setup makes OpenMC's core dependent on python, which is not ideal.

I suggest building NCrystal from source instead of relying on the ncrystal package. The issue with ncrystal is that it installs both ncrystal-core and ncrystal-python, and ncrystal-core includes libNCrystal.so, which is not used by libopenmc.so. Instead, libopenmc.so will use the libNCrystal.so copied to openmc.libs by auditwheel, leading to unnecessary duplication.

Let me know how you'd like to proceed.

@shimwell
Copy link
Owner

shimwell commented Feb 13, 2025

I don't think openmc will accept the PR if ncrystal is added as a build time or even as a dependency in the pyproject.toml file.

I agree with your recommendation of building NCrystal from source.

Just wondering if we can do the building NCrystal from source as a separate PR directly to openmc? and then once that is merged we come back to this packaging branch and benfit from those changes.

Are there any changes that can be done on the NCrystal side to makes this easier?

@shimwell
Copy link
Owner

shimwell commented Mar 6, 2025

I have merged in the recent changes that are now in openmc develop branch and solved merge conflicts.

I think we should build on this PR @ahnaf-tahmid-chowdhury and see if we can get a minimal scikit build core working then PR this branch into openmc

@tkittel
Copy link
Collaborator

tkittel commented Mar 6, 2025

I took a quick look at the "Files changed" (since I am looking forward to this PR!), and still see some ncrystal related stuff introduced. Is that needed any longer?

@shimwell
Copy link
Owner

shimwell commented Mar 6, 2025

I took a quick look at the "Files changed" (since I am looking forward to this PR!), and still see some ncrystal related stuff introduced. Is that needed any longer?

Do you have time to push ti this branch with the ncrystal related changes needed @tkittel

@tkittel
Copy link
Collaborator

tkittel commented Mar 6, 2025

Not at the moment unfortunately, I have some other issues I have to work on that I have postponed too many times :-)

But I simply meant that I don't think this PR need to add some NCrystal lib related stuff any longer, presumably? And looking at the diff it does add something.

@ahnaf-tahmid-chowdhury
Copy link
Collaborator

I am checking.

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.

4 participants