search

svalinn / DAGMC

Direct Accelerated Geometry Monte Carlo Toolkit
https://svalinn.github.io/DAGMC
Other
96 stars 61 forks source link

add packaging #910

Closed gonuke closed 9 months ago

gonuke commented 10 months ago

This adds the python packaging module to support updated pymoab installation in MOAB master branch.

Successful testing is demonstrated here.

shimwell commented 9 months ago

oh this is very cool, just repeating to check i have understood

the most recent versions of MOAB come with PyMoab that was causing a few installation issue. Consequently a few of us have stayed with MOAB 5.3.1 as the newest version that we have installed pymoab with.

However adding python3-packaging brings in the necessary components that make PyMoab installable again.

My only question is should python3-packaging should instead be added to the pymoab install process in the pyproject.toml build section?

perhaps to this location https://bitbucket.org/fathomteam/moab/src/aa7fca46f7f3d8d7d6cbadbc9b70f4fe29edec75/pymoab/pyproject.toml#lines-2

which already appears to have a packaging package in the list :eyes:

gonuke commented 9 months ago

Your understanding is correct (with the minor variation that DAGMC briefly worked with newer MOAB versions on 22.04 until the most recent update broke it again).

I am not sure how the pyptoject.toml is being used in the pymoab install process and why it's not being installed in an accessible way for DAGMC MOAB to access at build time.

Any ideas @pshriwise ?

gonuke commented 9 months ago

You can see a failure here: https://github.com/svalinn/DAGMC/actions/runs/6209143428/job/16856049951

shimwell commented 9 months ago

You can see a failure here: https://github.com/svalinn/DAGMC/actions/runs/6209143428/job/16856049951

I see it is running python setup.py and then failing with ModuleNotFoundError: No module named 'packaging' so this might bypass the use of the pyproject.toml file and the list of things to install contained within

I also notice there is a requirements.txt file which somewhat doubles up on the pyproject.toml as well https://bitbucket.org/fathomteam/moab/src/master/pymoab/requirements.txt

gonuke commented 9 months ago

I wonder if it needs to use pip install instead of python setup.py? I thought that the change to pip install had already happened.

pshriwise commented 9 months ago

@shimwell is right, we can do away with the requirements.txt file. The pyproject.toml isn't being copied to the build directory, which is where we call pip as part of the make install target, so the packaging build dependency isn't accounted for.

I'll submit a PR to MOAB shortly to fix this.

pshriwise commented 9 months ago

MOAB PR cros-reference: https://bitbucket.org/fathomteam/moab/pull-requests/651

vijaysm commented 9 months ago

In the future, we would appreciate it if you can raise issues or submit PRs when something fails in DagMC due to MOAB. This will help keep the library up to date in general as workflows evolve. @shimwell In general, I do not recommend sticking to old versions unless absolutely necessary.

gonuke commented 9 months ago

We are trying to get out of the habit of pinning to old versions @vijaysm but bandwidth limitations sometimes put us there. It's often not a MOAB bug/issue, but important progress in MOAB or other dependencies that we don't have the bandwidth to adapt to.

gonuke commented 9 months ago

MOAB PR cros-reference: https://bitbucket.org/fathomteam/moab/pull-requests/651

When this merges to MOAB/master, I can try our current test without this PR; if it works I'll withdraw this.

gonuke commented 9 months ago

It looks like develop passes now that MOAB master has been updated, so I'm withdrawing this PR

shimwell commented 9 months ago

we might be able to remove these from the dockerfile as well

                        python3-setuptools \
                        python3-dev \
                        libpython3-dev \