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

make mpie-cmti standalone #426

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

make mpie-cmti standalone #426

wants to merge 19 commits into from

Conversation

mbruns91
Copy link

This PR makes the mpie_cmti image definition "standalone", i.e. the layering starts with jupyter/base-notebook:ubuntu-22.0.

Before, the image was drived from pyiron, which was itself derived from md, which was derived from base, which was the only standalone image definition in docker-stacks.
Especially for this image, this makes managing the env easier, which is important as the shared conda environment on our cluster is based on this.

Broken down, I made some changes to Dockerfile, concatenating neccessary parts from the Dockerfiles of the previous "inheritance-chain". Similarly, I concatenated the environment.yml from all those image definitions

@mbruns91
Copy link
Author

mbruns91 commented Nov 29, 2024

@niklassiemer you think this is okay?

I started the container locally, ran mamba list and compared the output with the output of running the same command on cmti; they are the same.

@jan-janssen : Just added you to make you aware of this change.

Copy link
Member

@niklassiemer niklassiemer 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 should only see if this survives on the CI (probably larger?)

@mbruns91
Copy link
Author

Actually I think this image is smaller; I'm about to leave my desk, so I can check tomorrow or so.

The container started from the image defined here is basically empty.
Maybe (in another PR) we could add some notebooks for typical tests we do to check our environment. This way, one can do those checks easily from a local container instance. I'm thinking about the typical imports, plugins, ...; stuff like that.

@mbruns91 mbruns91 changed the title Mpie cmti standalone make mpie-cmti standalone Nov 29, 2024
Copy link
Member

@niklassiemer niklassiemer left a comment

Choose a reason for hiding this comment

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

We need to resync the dependencies. Some of them decreased, but probably I did not find all of them. E.g. it seems that bokeh and maxvolpy are not in the list any more? I will check again the next days :)

mpie_cmti/environment.yml Outdated Show resolved Hide resolved
mpie_cmti/environment.yml Outdated Show resolved Hide resolved
mpie_cmti/environment.yml Outdated Show resolved Hide resolved
mpie_cmti/environment.yml Outdated Show resolved Hide resolved
mpie_cmti/environment.yml Outdated Show resolved Hide resolved
mpie_cmti/environment.yml Outdated Show resolved Hide resolved
@mbruns91
Copy link
Author

mbruns91 commented Dec 4, 2024

Ok, so upon concatenating the environment.yml from the image definitions of pyiron, base and md there occurred some merge conflict. Upon resolving this I seem to have removed some packages accidentially... I hope the last commit fixed that.

@mbruns91
Copy link
Author

mbruns91 commented Dec 4, 2024

Environment-wise everything seems to be okay now. But now the image(s) are too large.
The mpie_cmti image is not actrually too large per se. But now, as it's "standalone", it doesn't share any layers with the other images anymore, so the device volume (the runners diks space) is filled faster.
This was already an issue in the past, right? I think we could circumvent this, if not all steps of the building/testing were performed on the same runner instance.

We could either

  • try to only build & test the images derived from the files on the affected directory of a PR (e.g. in this case mpie_cmti only). I think this could be a bit ticky but doable. However it would increase the complexity of the CI/CD pipeline.
  • make seperate repos for each image (and make "docker-stacks" an organization?).

What do you think, @niklassiemer?

@niklassiemer
Copy link
Member

I would not make an org for such few repos, and I also do not even think we need multiple repos. For now, we can "just" use different CI runs, one for each scenario. The main thing would be how to build and push all docker images in case of a release... maybe we need to split the releases like "cmti_mpie_${date}" release instead of only "${date}" ones

@niklassiemer
Copy link
Member

Now I tried to split the testing to two VMs like before in #347. Back then I failed (and the issue was not to pressing) since all the builds needed to be in one VM. Now, this is not intended this way any longer!

@mbruns91 mbruns91 removed the request for review from jan-janssen December 6, 2024 12:05
@mbruns91
Copy link
Author

mbruns91 commented Dec 6, 2024

Let's merge this PR and do the splitting for the test and release in another PR. @niklassiemer , can you please approve the changes?

@niklassiemer
Copy link
Member

Hmm... the code in test and push is quite similar and I would expect the push to break due to the changes in the cmti/Dockerfile. Thus, let me take over the responsibility and I try to get the release action aligned with the test action before merging it in.

@niklassiemer niklassiemer self-assigned this Dec 6, 2024
@mbruns91
Copy link
Author

mbruns91 commented Dec 6, 2024

Yeah, you're right. You really want to do it? I can take care of it as well.

@niklassiemer
Copy link
Member

Ok, if you want to play a bit with the github CI, go ahead :) There are actions using caching and or artifacts which can be used to combine things from different jobs. Just ping me for any question 👍

@niklassiemer niklassiemer removed their assignment Dec 6, 2024
@mbruns91 mbruns91 self-assigned this Dec 6, 2024
@mbruns91 mbruns91 mentioned this pull request Dec 6, 2024
@mbruns91
Copy link
Author

mbruns91 commented Dec 6, 2024

I really don't like to do all of this in this PR, it appears messy to me.

I opened another (draft) PR where I'm working on the CI improvements and applied the same changes done here to testing.yml.

@niklassiemer
Copy link
Member

The problem is a bit that these things are indeed coupled... i.e. you need the Dockerfile change to build standalone and you need the ci change that this fits into one VM... probably we need stacked PRs to have it clean :)

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.

2 participants