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

DevOps: Add devcontainer #5913

Merged
merged 19 commits into from Mar 12, 2023
Merged

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented Mar 5, 2023

The devcontainer specification [1] is about making it easier for developers to get a first development environment up and running.

This gives (prospective) AiiDA developers and contributors a fully-fledged AiiDA development environment in two ways:

  • Zero-install with Codespaces in the web browser.
    On GitHub, a user can simply click on "Code" => "Open with Codespaces", wait 5 minutes
  • One-command cross-platform with VS Code + Docker.
    In the command palette, type "Open Folder in Container" (automatically pops up when opening folder in VS Code)

[1] https://containers.dev

@ltalirz
Copy link
Member Author

ltalirz commented Mar 5, 2023

a short video of how it works https://www.youtube.com/watch?v=ibkFSNLQsww&ab_channel=LeopoldTalirz

you can try it yourself at https://github.com/ltalirz/aiida-core/tree/feat/devcontainer-compose

Note: I first built a version with a single container in https://github.com/ltalirz/aiida-core/tree/feat/devcontainer but then abandoned it since the approach of the /sbin/my_init script does not work well together with the way VS Code treats the container.-

@ltalirz
Copy link
Member Author

ltalirz commented Mar 5, 2023

the duplication of the configure-aiida.sh can be removed before merging this;
it is only necessary because I'm using the aiida-core:main image as a base for the aiida service to speed up build time.

@ltalirz ltalirz marked this pull request as draft March 5, 2023 22:43
@ltalirz
Copy link
Member Author

ltalirz commented Mar 5, 2023

@yakutovicha I understand that the aiida-core Docker container is no longer used in the AiiDA lab, is that correct?

If that is the case, then we could probably at some point get rid of the aiida-prerequisites container in favor of a docker-compose.yml like the one I'm adding here that uses separate containers for postgres & rabbitmq.
The Dockerfile in the aiida-core repo could become self-contained and simply derive from something like python:3.9.

@yakutovicha
Copy link
Contributor

@yakutovicha I understand that the aiida-core Docker container is no longer used in the AiiDA lab, is that correct?

That is correct 👍

@ltalirz
Copy link
Member Author

ltalirz commented Mar 7, 2023

@unkcpz @chrisjsewell @yakutovicha any thoughts on this PR?

It anyhow reuses the current aiida-core container, so the decision of deprecating aiida-prerequisites or not can be decoupled.

@chrisjsewell
Copy link
Member

Looks fine to me, I've always championed multiple containers (via docker-compose) and VS Code

@unkcpz
Copy link
Member

unkcpz commented Mar 7, 2023

The Dockerfile in the aiida-core repo could become self-contained and simply derive from something like python:3.9.

I fully agree with this. I think instead of having the aiida image in aiidalab as I tried in aiidalab/aiidalab-docker-stack#319, it makes more sense to just have the parallel one base on the simple python base image for aiida. This may increase a bit duplication between aiidalab-docker-stack and the image in aiida-core, but that will make the images have clear usage for users and can be found from the corresponding repository.

Comment on lines +33 to +36
aiida:
#image: "aiidateam/aiida-core:main"
image: "aiida-core-dev"
build:
Copy link
Member

Choose a reason for hiding this comment

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

I think the image aiidalab/base from https://hub.docker.com/r/aiidalab/base/tags is what we can put here in the future if we manage to move it from aiidalab to here, correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Almost. It would probably rather become FROM image in the .devcontainer/Dockerfile, since the devcontainer focuses on development (which requires e.g. some further dependencies installed).

@ltalirz
Copy link
Member Author

ltalirz commented Mar 7, 2023

Thanks for the feedback @unkcpz !

Given this, I will prepare this PR for review with minimal changes;
in a follow-up PR we can move from the current Dockerfile in the root of the repo to a more minimal one like the one you drafted in aiidalab/aiidalab-docker-stack#319

@ltalirz ltalirz requested a review from unkcpz March 8, 2023 14:24
@ltalirz
Copy link
Member Author

ltalirz commented Mar 8, 2023

PR is now ready for review (the UI doesn't let me convert it from a draft)

@chrisjsewell chrisjsewell marked this pull request as ready for review March 8, 2023 15:31
Copy link
Member

@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.

thanks @ltalirz, I just have one question. I'll test locally it later.

Comment on lines 56 to 57
# volumes:
# db-volume:
Copy link
Member

Choose a reason for hiding this comment

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

Do this need to be kept here? Since it is useful for having data stored persistent I suggest keeping it and adding more details.

Copy link
Member Author

@ltalirz ltalirz Mar 9, 2023

Choose a reason for hiding this comment

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

Yes, that was the intention. Will add further comments

Copy link
Member Author

Choose a reason for hiding this comment

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

in the end I decided to remove them as they are untested.

it's easy enough to reintroduce them and go to a setup with persistent data

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, but it is a bit confusing for me how this codespace is working, if I understand correctly, the container is opened in a remote machine provided by GitHub. How is this persistent volumes mapping can mapping to the local DB file or local file repository?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed this would have been intended for running the devcontainer locally through VS Code.

I am not aware of a way of persisting files after github codespaces deletes the containers.

remove comments
@ltalirz ltalirz requested a review from unkcpz March 12, 2023 00:28
Copy link
Member

@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.

@ltalirz Thanks for the update. I give it a test and it is super cool. Just one follow-up question on the volume mapping of the container.

@ltalirz ltalirz merged commit 4010cc9 into aiidateam:main Mar 12, 2023
10 checks passed
@ltalirz ltalirz mentioned this pull request Mar 12, 2023
4 tasks
@sphuber sphuber deleted the feat/devcontainer-compose branch March 13, 2023 07:53
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.

None yet

4 participants