Skip to content

PyPI support #1

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

Merged
merged 9 commits into from
May 16, 2025
Merged

PyPI support #1

merged 9 commits into from
May 16, 2025

Conversation

ahnaf-tahmid-chowdhury
Copy link
Member

@ahnaf-tahmid-chowdhury ahnaf-tahmid-chowdhury commented Nov 11, 2023

Description

This PR introduces automated PyPI package publishing through GitHub Actions workflows. This addition streamlines the release process, ensuring consistent and reliable distribution of the package.

Sponsored By


This contribution is kindly sponsored by @proximafusion, supporting the ongoing development and modernization of DAGMC’s Python packaging and build system.

@pshriwise
Copy link
Member

Sorry for the delayed reply here, @ahnaf-tahmid-chowdhury. We're getting closer, but I'm not sure we're quite ready for PyPI. Let's revisit this soon!

@gonuke
Copy link
Member

gonuke commented Mar 4, 2024

Maybe it would be helpful to update these actions to do all the testing and staging, but not actually publish to PyPI? Then it will be ready when needed. There is clearly some work to do to figure out how to bundle MOAB for PyPI, as well.

@ahnaf-tahmid-chowdhury
Copy link
Member Author

The PyPI workflow only runs when we create a new tag; otherwise, it will not run. However, there is another step to consider when working with PyPI: we need to configure the environment on the PyPI side, or else the workflow will fail to publish on PyPI.

I am a little worried when talking about MOAB pip support. I have already opened PRs in the MOAB repo, which are still under review (Inactive). To implement the scikit build method, we must first consider fixing the Windows build.

@gonuke
Copy link
Member

gonuke commented Mar 5, 2024

I'm not optimistic about those changes in MOAB as they are focused on their core community which is broader than us and generally not on a Windows system

@ahnaf-tahmid-chowdhury
Copy link
Member Author

A PR is added to MOAB

@ahnaf-tahmid-chowdhury
Copy link
Member Author

Is HDF5 a runtime dependency of DAGMC or it is only required at build time? If so, we may need to enable PyPI support for it as well.

@gonuke
Copy link
Member

gonuke commented Mar 18, 2024

libhdf5 is a runtime dependency. I don't think it's in our scope or mandate to add PyPI support for HDF5

@gonuke
Copy link
Member

gonuke commented Mar 18, 2024

I think depending on h5py may take care of it??

@ahnaf-tahmid-chowdhury
Copy link
Member Author

The following libraries are included with h5py and stored in site-packages/h5py.libs:

  • libaec-001fb5f0.so.0.0.12
  • libhdf5-7f639dcd.so.310.2.0
  • libhdf5_hl-123198ff.so.310.0.2
  • libsz-b66d1717.so.2.0.1

That's mean we can stay with h5py. Maybe need an update to FindHDF5 file?

@pshriwise
Copy link
Member

To simplify things here, let's separate some concerns. From the perspective of this project, I don't think we should concern ourselves at all with how pymoab is packaged -- only that it's present for pydagmc as a dependency.

@ahnaf-tahmid-chowdhury, the set of changes here does a few things that I'd like to break into multiple smaller PRs.

  1. A PR updating the pyproject.toml with the changes here. They're mostly good but I have some comments specific to that file.
  2. A PR adding the workflor for PyPI packaging and release, but without the MOAB build. I don't see why we're building and packaging MOAB/pymoab in that workflow, but let me know if I'm missing something there.
  3. A PR with the test directory updates (this could optionally be wrapped into the first PR if you like).

One last small thing. I do prefer we keep the GH workflow for testing named ci.yml. This is, admittedly. a little selfish -- almost all of the other projects I work on use this name and I like the convention. If you have some particular motivation for changing the name of that workflow @ahnaf-tahmid-chowdhury, let me know.

@ahnaf-tahmid-chowdhury
Copy link
Member Author

Absolutely, I'm fine with creating multiple PRs. However, at this moment, we're not prepared to publish our project on PyPi. Our project heavily relies on pymoab, which we import in our main file. Therefore, pymoab needs to be available on PyPi first.

@pshriwise
Copy link
Member

pshriwise commented May 24, 2024

Absolutely, I'm fine with creating multiple PRs. However, at this moment, we're not prepared to publish our project on PyPi. Our project heavily relies on pymoab, which we import in our main file. Therefore, pymoab needs to be available on PyPi first.


@ahnaf-tahmid-chowdhury, @paulromano @gonuke and I have been discussing this offline. I'll summarize that discussion here so we're all on the same page. There are two approaches we've considered:

  1. Publish on PyPI knowing that pymoab isn't available.

This goes against convention for packages on PyPI, but it isn't unheard of. https://github.com/Thea-Energy/stellarmesh does this, for example.

Upsides: Users learn early (before they try to run a script or import pydagmc) that they need an additional dependency, and, if we setup the packaging correctly, nothing has to change in this repo once pymoab is on PyPI.

Downsides: Goes against convention and it isn't immediately clear to the person installing how to proceed in installing pymoab.

  1. Make pymoab a runtime dependency of pydagmc. This means we'd remove pymoab as a requirement in the pyproject.toml file and perform a check for pymoab internally in pydagmc. The packages https://github.com/fusion-energy/cad_to_dagmc and https://github.com/openmsr/CAD_to_OpenMC handle it this way. Something like:
try:
    import pymoab
except ImportError as e:
    # print message about missing pymoab and point to installation instructions/MOAB docs

Upsides: PyPI installation of pydagmc is smoother, even without pymoab available. This path also allows us to provide more information to the person installing on how to proceed with installing pymoab .

Downsides: The user potentially won't know about the missing dependency until they try to run code that imports pydagmc (definition of a runtime dependency I know, but still a tradeoff with option 1 in my mind).


All things considered I think option 2 is the most sensible for now.

@ahnaf-tahmid-chowdhury
Copy link
Member Author

Following option 2, I also think this one is the best. I will add some instructions for installing pymoab in the comments.

@pshriwise
Copy link
Member

Thanks for being willing to take that on! Let's make the transition of pymoab to a runtime dependency a separate PR if you would. Smaller PRs are much easier for us to review and merge quickly.

@pshriwise
Copy link
Member

Also, I think we'd ideally provide a link to full instructions for pymoab installation within MOAB, but if those don't exist then housing them here temporarily might make sense.

@jon-proximafusion
Copy link

jon-proximafusion commented Oct 14, 2024

Also, I think we'd ideally provide a link to full instructions for pymoab installation within MOAB, but if those don't exist then housing them here temporarily might make sense.

I think the pip install for pymoab is now just a clone and pip install command, thanks to @ahnaf-tahmid-chowdhury

git clone  master https://bitbucket.org/fathomteam/moab/
cd moab
# hdf5 is needed to be installed separately at the moment 
sudo apt-get install libhdf5-dev
# ensure pip is up to date as new version is needed
python -m pip install --upgrade pip
pip install . --config-settings=cmake.args=-DENABLE_HDF5=ON

@MicahGale
Copy link

I just wanted to check in: what's the status of this? We were just discussing I do a PR like this to support parastell, so seeing as most of the work has been done is great.

@MicahGale
Copy link

@pshriwise this seems pretty ready to me. Could you check when you have a moment?

@pshriwise
Copy link
Member

As far as I can tell, I think we just need to clean up the workflow -- dispatching to the test platform vs. PyPI itself depending on the workflow trigger event.

Not complaining as it gives me some time to think about adding documentation/examples before we release :)

Copy link

codecov bot commented May 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.19%. Comparing base (5d38439) to head (123dfe5).
Report is 11 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main       #1   +/-   ##
=======================================
  Coverage   97.19%   97.19%           
=======================================
  Files           2        2           
  Lines         428      428           
  Branches       47       47           
=======================================
  Hits          416      416           
  Misses          9        9           
  Partials        3        3           
Flag Coverage Δ
unittests 97.19% <ø> (ø)

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@pshriwise pshriwise left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you @ahnaf-tahmid-chowdhury!

@shimwell
Copy link
Member

LGTM

Copy link

@MicahGale MicahGale left a comment

Choose a reason for hiding this comment

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

LGTM. We were able to discuss how to use environment protections to my satisfaction.

@pshriwise pshriwise merged commit 9c82e1d into svalinn:main May 16, 2025
3 checks passed
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.

6 participants