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

PyPI support #1

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ahnaf-tahmid-chowdhury
Copy link
Member

Workflows added for testing
Workflows added for publish to PyPI on tag release
Dynamic version set on tag release

@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

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