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

add packaging #910

Closed
wants to merge 2 commits into from
Closed

add packaging #910

wants to merge 2 commits into from

Conversation

gonuke
Copy link
Member

@gonuke gonuke commented Sep 28, 2023

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

Successful testing is demonstrated here.

@gonuke gonuke requested review from pshriwise and shimwell October 4, 2023 12:03
@shimwell
Copy link
Member

shimwell commented Oct 4, 2023

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 👀

@gonuke
Copy link
Member Author

gonuke commented Oct 4, 2023

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
Copy link
Member Author

gonuke commented Oct 4, 2023

CI/Dockerfile Outdated
@@ -41,6 +41,7 @@ RUN apt-get -y update; \
python3 \
python3-pip \
python3-setuptools \
Copy link
Member

Choose a reason for hiding this comment

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

setuptools and packaging are mentioned in the pyproject.toml of pymoab so ideally they would have been installed by python during the instal process.

I see adding them to the docker works which is great but I would be keen to understand why they are not being installed automatically like other build dependencies

@shimwell
Copy link
Member

shimwell commented Oct 4, 2023

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
Copy link
Member Author

gonuke commented Oct 4, 2023

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
Copy link
Member

@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
Copy link
Member

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

@vijaysm
Copy link
Contributor

vijaysm commented Oct 5, 2023

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
Copy link
Member Author

gonuke commented Oct 5, 2023

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
Copy link
Member Author

gonuke commented Oct 5, 2023

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.

CI/Dockerfile Outdated
@@ -41,6 +41,7 @@ RUN apt-get -y update; \
python3 \
python3-pip \
python3-setuptools \
python3-packaging \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
python3-packaging \

shall we remove this and retrigger the CI

Copy link
Member

Choose a reason for hiding this comment

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

🤞🏻

@gonuke
Copy link
Member Author

gonuke commented Oct 12, 2023

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

@gonuke gonuke closed this Oct 12, 2023
@shimwell
Copy link
Member

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

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

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