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

AEP: new docker stack to handle services #39

Merged
merged 1 commit into from
Sep 7, 2023

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Jul 16, 2023

  • Used AEP template from AEP 0
  • Status is draft
  • Added type & status labels to PR
  • Added AEP to README.md
  • Provided github handles for authors

@unkcpz
Copy link
Member Author

unkcpz commented Jul 16, 2023

@unkcpz unkcpz requested a review from sphuber July 16, 2023 21:12
@sphuber
Copy link
Contributor

sphuber commented Sep 6, 2023

Thanks @unkcpz . I have read through the AEP and most of it seems good to me. I have added a commit with some small fixes to readability. This was easier than doing it through suggestions through the PR review interface. Feel free to point out changes that are incorrect so we can fix them.

I have one major question I think: I am bit confused on the image names. It seems that the docker files in aiida-core are called base and base-with-service. But the names on docker hub are supposed to be base and aiida-core? Why are they different? And would base and full be simpler and convey just as much? And do we need to prefix them with aiida- to make clear that they are images for AiiDA? E.g., aiida-base and aiida-full. Or is the context of the image always clear?

Copy link
Member Author

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

@sphuber thanks a lot for the correction, two places may change what I want to express initially.

It seems that the docker files in aiida-core are called base and base-with-service. But the names on the docker hub are supposed to be base and aiida-core?

I kept naming the image in ghcr and dockerhub the aiida-core since I thought it was more natural for people who already use the aiidateam/aiida-core image, no?

The base-with-services is directly from aiidalab, I think the name explains what it is. The full seems a bit too far, what if we want to have an image that includes all the developing tools installed in the image? I have no preference for adding aiida- in front. Maybe not be necessary since for the image we already own the aiidateam and aiidalab namespace.

I see one downside of using name without aiida- that is when the image is used as a base image by other people and upload to dockerhub, if they keep using base would make no sense. So what about we name these two aiidateam/aiida-base and aiidateam/aiida-core (If the first point I mention is not important, aiida-full is also an option)?
Since these two were intended to replace aiidalab/base and aiidalab/base-with-services, maybe following the naming convention is self-explained by name thus a good option as well.

- Useless to use `jupyter/minimal-notebook` as base image

The `jupyter/minimal-notebook` will mapping the port and start a jupyter notebook as the main service of the container which for the `aiida-core` user it is useless.
Due to the switch to `jupyter/minimal-notebook` as the base image, the `aiida-prerequisites` image has become superfluous and they no longer rely on the `aiidateam/aiida-core` image.
Copy link
Member Author

Choose a reason for hiding this comment

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

It was aiidateam/aiida-core image rely on the aiida-prerequisites

Copy link
Contributor

Choose a reason for hiding this comment

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

It is indeed worded a bit unclear. With the "they no longer rely" I meant the "they" to refer to the aiidalab organization, which now just uses jupyter/minimal-notebook. Will improve the language a bit


5. Hide `base` and `base-with-services` images from docker hub and ghcr.
### Hide `base` and `base-with-services` images from docker hub and ghcr
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry that I confused you her. This is a 5th point and the goal was to not show base and base-with-services images for aiidalab. In the end, we will have four official maintained images:

  • aiidateam/base
  • aiidateam/aiida-core
  • aiidalab/lab
  • aiidalab/full-stack

Copy link
Contributor

Choose a reason for hiding this comment

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

This point used to be in the "goals" section, but since it really is more of an action I have moved it to the "Proposed enhancement". The goal is to make the available images more clear, and this is part of the solution, by hiding certain base images that should not be of interest to users.

Is that interpretation correct?

Maybe I can then simply add a sentence to this section that explicitly says that these images will be hidden on docker hub because they should not be useful to users and can simply confuse.

Copy link
Contributor

Choose a reason for hiding this comment

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

And once more regarding the naming:

  • aiidateam/base
  • aiidateam/aiida-core

When I see that, to me it is not really clear what each image provides. The base image might be problematic, because it is not clear that it is for AiiDA. Note that we currently don't have other projects in aiidateam that provide a Docker image, but that could be in the future, and users might not know. So I think we should prefix it with aiida-. But also when we stick with base and aiida-core, which one contains the services? To me it is not clear.

@sphuber
Copy link
Contributor

sphuber commented Sep 7, 2023

I see one downside of using name without aiida- that is when the image is used as a base image by other people and upload to dockerhub, if they keep using base would make no sense. So what about we name these two aiidateam/aiida-base and aiidateam/aiida-core

I think we should definitely prefix with aiida- for the reasons I explained in another comment but yours is also important (what happens if the image is taken out of the aiidateam namespace, base is too generic). We can bring it up during the team meeting today. I feel aiida-base and aiida-core are too close and to me it is not clear what the difference would be. Maybe aiida-core-base and aiida-core-services ? Since the difference is that one image is a base image and the other one includes the services.

@sphuber
Copy link
Contributor

sphuber commented Sep 7, 2023

After discussion during the team meeting, we settled on aiida-core and aiida-core-with-services for the names of the two images.

@unkcpz I will update the AEP, can you take care of updating the PR on aiida-core? Then I will take care of merging this

@unkcpz
Copy link
Member Author

unkcpz commented Sep 7, 2023

I will update the AEP, can you take care of updating the PR on aiida-core? Then I will take care of merging this

Sure, I'll update the PR. Thanks for taking care of AEP.

The current Docker image provided with `aiida-core` depends on
`aiida-prerequisites` as a base image. This image is maintained outside
of the `aiida-core` repo, making it additional maintenance to keep it up
to date when a new `aiida-core` version is released. In addition, the
`aiida-prerequisites` image is no longer maintained because the AiiDAlab
stack now depends on another base image.

Finally, the `aiida-prerequisites` design had shortcomings as to how the
required services, PostgreSQL and RabbitMQ, are handled. They had to be
started manually and were not cleanly stopped on container shutdown.

Here a proposal is submitted to add two Docker images to `aiida-core`
that simplifies their maintenance and that improve the usability by
properly and automatically handling the services.
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks a lot @unkcpz great work!


5. Hide `base` and `base-with-services` images from docker hub and ghcr.
### Hide `base` and `base-with-services` images from docker hub and ghcr
Copy link
Contributor

Choose a reason for hiding this comment

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

This point used to be in the "goals" section, but since it really is more of an action I have moved it to the "Proposed enhancement". The goal is to make the available images more clear, and this is part of the solution, by hiding certain base images that should not be of interest to users.

Is that interpretation correct?

Maybe I can then simply add a sentence to this section that explicitly says that these images will be hidden on docker hub because they should not be useful to users and can simply confuse.


5. Hide `base` and `base-with-services` images from docker hub and ghcr.
### Hide `base` and `base-with-services` images from docker hub and ghcr
Copy link
Contributor

Choose a reason for hiding this comment

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

And once more regarding the naming:

  • aiidateam/base
  • aiidateam/aiida-core

When I see that, to me it is not really clear what each image provides. The base image might be problematic, because it is not clear that it is for AiiDA. Note that we currently don't have other projects in aiidateam that provide a Docker image, but that could be in the future, and users might not know. So I think we should prefix it with aiida-. But also when we stick with base and aiida-core, which one contains the services? To me it is not clear.

@sphuber sphuber merged commit 6cb8a28 into aiidateam:master Sep 7, 2023
@unkcpz unkcpz deleted the new-docker-stack branch September 14, 2023 13:15
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