Skip to content

Provide a minimal LHAPDF container#3

Merged
alecandido merged 4 commits into
mainfrom
lhapdf-container
Oct 4, 2022
Merged

Provide a minimal LHAPDF container#3
alecandido merged 4 commits into
mainfrom
lhapdf-container

Conversation

@alecandido
Copy link
Copy Markdown
Collaborator

@alecandido alecandido commented Sep 23, 2022

For other projects sake, here I provide a minimal LHAPDF container, with the library and Python package pre-installed.
The container is published at:

https://github.com/N3PDF/workflows/pkgs/container/lhapdf

It already contains a few common PDF sets pre-installed, with a possibly unopinionated (though small) selection:
https://github.com/N3PDF/workflows/blob/4a3f927ceef7b6df797bf02f1d333c41a58fd934/containers/lhapdf/Containerfile#L38-L44

@alecandido
Copy link
Copy Markdown
Collaborator Author

There is a ongoing issue with duplication of installation scripts on another repo.
The issue will be addressed on the other side.

@Radonirinaunimi
Copy link
Copy Markdown
Member

There is a ongoing issue with duplication of installation scripts on another repo. The issue will be addressed on the other side.

This is indeed the downside part. Until we find cleaner alternatives this should do the job. I will have a look a bit later (we still have a bunch of meetings this afternoon).

@Radonirinaunimi
Copy link
Copy Markdown
Member

@alecandido, in order for this to be used, one also needs to pass the container image to the on.workflow_call.inputs?

container-image:
     type: string
     required: false

@alecandido
Copy link
Copy Markdown
Collaborator Author

We can try. So, if you feel like it is easily doable, you can propose a PR.

On my side, I'd rather keep it simple, even though it is not too flexible. In case the reusable workflow is not good enough, write your own (I know we want to reduce duplication, that's the goal of this repo in the first place, but I'd like to avoid doing it at the price of maintainability).

@felixhekhorn
Copy link
Copy Markdown
Collaborator

What should I do here?

@alecandido
Copy link
Copy Markdown
Collaborator Author

What should I do here?

Check if things make sense, and in case of extremely good will, try to run the container yourself :)

name: 🔬 Test (🐍 ${{ inputs.python-version }})
runs-on: ubuntu-latest
container:
image: ${{ inputs.container-image }}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

What happens if no container-image variable is set?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In principle, if the variable is empty it just skips it. I was about to confirm this next but it looks like the docker pulls fail https://github.com/NNPDF/nnusf/actions/runs/3138407519/jobs/5097703520

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should perhaps remove the authentication when pulling the image?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There is no authentication: it is a public package, and the workflow should be able to access it without further ado.

Here it is actually working: https://github.com/N3PDF/pineappl/blob/master/.github/workflows/wheels.yml

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is what I thought as well but it looks like one needs to authenticate to the container registry. Otherwise, I don't know what's going on.

If you try to run the action and it doesn't work then it must be something else.

Copy link
Copy Markdown
Collaborator

@felixhekhorn felixhekhorn 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! I trust in @Radonirinaunimi to try the image (as he seems to do) ;-)

concerning the duplication: can't you clone the external repo inside?

@alecandido
Copy link
Copy Markdown
Collaborator Author

alecandido commented Sep 28, 2022

concerning the duplication: can't you clone the external repo inside?

Indeed, but most likely I'll do the other way round, since this is public and the other private (otherwise people could not get the proper repo here).

The problem is that the proper way of "cloning" is a submodule. It is an option, and it is well-maintained by Git, but from first-hand experience (and you were with me), submodules are not trivial. So, I was waiting one moment to see if I could come with a better idea, but I'm more and more convinced that a submodule is the proper way to deal with this...

@alecandido
Copy link
Copy Markdown
Collaborator Author

Duplication problem solved: we are going to kill N3PDF/external.

I'm importing everything else in #4, but before I'd like to merge this.

@felixhekhorn
Copy link
Copy Markdown
Collaborator

since nothing changed since last I reviewed there's nothing I can add 🙃 feel free to merge

@alecandido
Copy link
Copy Markdown
Collaborator Author

since nothing changed since last I reviewed there's nothing I can add upside_down_face feel free to merge

Conditions are now different, I was checking someone still agreed :)

@alecandido alecandido merged commit 9a851cc into main Oct 4, 2022
@alecandido alecandido deleted the lhapdf-container branch October 4, 2022 15:34
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.

3 participants