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

Dev containers #1228

Merged
merged 57 commits into from
Dec 23, 2024
Merged

Dev containers #1228

merged 57 commits into from
Dec 23, 2024

Conversation

drifter089
Copy link
Contributor

Closes # (if applicable).

Changes proposed in this Pull Request

Checklist

  • I consent to the release of this PR's code under the AGPLv3 license and non-code contributions under CC0-1.0 and CC-BY-4.0.
  • I tested my contribution locally and it seems to work fine.
  • Code and workflow changes are sufficiently documented.
  • Newly introduced dependencies are added to envs/environment.yaml and doc/requirements.txt.
  • Changes in configuration options are added in all of config.default.yaml and config.tutorial.yaml.
  • Add a test config or line additions to test/ (note tests are changing the config.tutorial.yaml)
  • Changes in configuration options are also documented in doc/configtables/*.csv and line references are adjusted in doc/configuration.rst and doc/tutorial.rst.
  • A note for the release notes doc/release_notes.rst is amended in the format of previous release notes, including reference to the requested PR.

drifter089 and others added 30 commits October 10, 2024 17:39
…ets-earth#1134)

* fix issue of adding H2 store and make H2 Store in AC buses

* add release notes

* Add H2 Stores to both AC and DC buses
updates:
- [github.com/pre-commit/pre-commit-hooks: v4.6.0 → v5.0.0](pre-commit/pre-commit-hooks@v4.6.0...v5.0.0)
- [github.com/psf/black: 24.8.0 → 24.10.0](psf/black@24.8.0...24.10.0)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
…#1135)

* enable configfile for mock_snakemake

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add release notes

* drop else condition

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@drifter089
Copy link
Contributor Author

huge shoutout to @GbotemiB for his contributions
I'm grateful for the positive feedback on our work,

@ekatef
Copy link
Member

ekatef commented Dec 11, 2024

huge shoutout to @GbotemiB for his contributions I'm grateful for the positive feedback on our work,

Kudos for the fantastic collaborative work to you, @GbotemiB and @lkstrp 😄

@ekatef
Copy link
Member

ekatef commented Dec 11, 2024

Hello everyone, thanks for the great work @drifter089 ! Just to chime in here: As discussed with Akshat:

we are thinking of triggering container build when pinned-envs is updated

  • I would argue that the dev container images should only be triggered by changes to the pinned envs. Since there is no package data in those images, the pinned env will be the only difference between the images, and we don't need to rebuild and push if they are the same.
  • For a bright future where we might have a GUI/full fletched app image, we'd like to keep the pypsa-earth package name for that. Maybe pypsa-earth-dev-env instead for the image?

We will implement a similar structure for pypsa-eur and should decide on the same naming pattern to make it less confusing for any user. Also, we'll want to use these images for dev and validation runs, so when we bring them to Earth (#1164) it'll be easier if the setup is the same.

Thank you so much @lkstrp for supporting it!! Having a container image will be a fantastic improvement.

Adding some thoughts, feel free to comment and disagree 😄

  • Currently, the pinned envs are being updated once per week which usually leads to some changes in the pinned environments. So, it looks to me as the approaches suggested by you and @drifter089 are basically equivalent

  • Do you mean as full-fletched that the docker image may also include all the data needed to run a model? That would be a really amazing! If we would continue to dream, a dedicated image could be created for each continent. It would take ~6 * 30 Gb storage, but would be definitely worth it.

With a question on the naming, you risk to open Pandora box 😄 My understanding is that the current approach is intended for usage rather by data scientists/modellers who are relatively comfortable with containers, but are not particularly keen to go trough seven circles of pythonic dependency management 🙃 Though, pypsa-earth-dev-env sounds nice, and I think it's more for @GbotemiB and @drifter089 to discuss the name as having the docker image is their result 🎊 🚀 ⭐

@ekatef
Copy link
Member

ekatef commented Dec 11, 2024

Btw, in puncto dependencies management: @lkstrp haven't you noticed any troubles related to pip-dependencies when generating pinned environments?

I have an impression that there may be some troubles in how conda interact with pip which regularly sometimes lead to the need for manual interventions. Would be actually great to find a way to make the automated workflow work automatically 😄 Any hints on that would be highly appreciated!

@lkstrp
Copy link
Contributor

lkstrp commented Dec 12, 2024

Do you mean as full-fletched that the docker image may also include all the data needed to run a model? That would be a really amazing! If we would continue to dream, a dedicated image could be created for each continent. It would take ~6 * 30 Gb storage, but would be definitely worth it.

Naa, no data, just source code. The dev envs do not even contain that at the moment. Source code and possibly a GUI at some point, so you could run a full fletched app with a single command.

Btw, in puncto dependencies management: @lkstrp haven't you noticed any troubles related to pip-dependencies when generating pinned environments?

Not so far, no. The main problem I have encountered is that you need OS specific pinned envs, but this is being handled. So far the pinned ones work quite well, which doesn't mean there aren't problems 😄

The other points I discussed with Akshat.

@ekatef ekatef added this to the Version 1.0 milestone Dec 14, 2024
Copy link
Member

@davide-f davide-f left a comment

Choose a reason for hiding this comment

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

Great PR :D
Honestly, it is a great contribution and in amazing status.

On my side, I'd have only a minor fix (the tutorial test), plus the release note.
A test on the installation of the docker could be interesting too, we can have it in a future PR in case.

config.tutorial.yaml Outdated Show resolved Hide resolved
@davide-f
Copy link
Member

@ekatef I believe you have handled it more then me; if it is fine, I leave it to you :)

Copy link
Member

@ekatef ekatef left a comment

Choose a reason for hiding this comment

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

Hello @drifter089 @GbotemiB, fantastic work!! My feeling is that we are close to finalise 🚀

Have added some comments which are mainly quite minor. My only major concern related to the overall architecture to manage docker infrastructure. Would it be possible to use pypsa-earth directly to store the image and clone the repo? Obviously, we would need you support for that. May you be willing to take the lead in managing dockerisation directly in the upstream?

Generally, the contribution feels like a great usability improvement, and looking forward to merge it 😄

Comment on lines +6 to +9
"name": "pypsa earth dev",
"image": "ghcr.io/drifter089/pypsa-earth:latest",
"workspaceMount": "source=${localWorkspaceFolder},target=/workspaces,type=bind,consistency=cached",
"initializeCommand": "docker pull ghcr.io/drifter089/pypsa-earth:latest",
Copy link
Member

Choose a reason for hiding this comment

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

@drifter089 I understand that it has been crucial to test the docker workflow, and thanks a lot for making it work using your personal repo! For the stable implementation, I wonder if can we use pypsa-earth repo. Obviously, happy to provide you with all the administrative rights to maintain the docker images. Could it work?

.devcontainer/welcome-message.txt Outdated Show resolved Hide resolved
Docker.MD Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
config.tutorial.yaml Outdated Show resolved Hide resolved
.devcontainer/welcome-message.txt Outdated Show resolved Hide resolved
.github/workflows/devcontainer.yml Outdated Show resolved Hide resolved
Docker.MD Outdated Show resolved Hide resolved
doc/docker_containers.rst Show resolved Hide resolved
Copy link
Contributor

@GbotemiB GbotemiB left a comment

Choose a reason for hiding this comment

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

Hi @ekatef, thank you for taking time to review. I have completed the requested changes.

@GbotemiB
Copy link
Contributor

Hello @drifter089 @GbotemiB, fantastic work!! My feeling is that we are close to finalise 🚀

Have added some comments which are mainly quite minor. My only major concern related to the overall architecture to manage docker infrastructure. Would it be possible to use pypsa-earth directly to store the image and clone the repo? Obviously, we would need you support for that. May you be willing to take the lead in managing dockerisation directly in the upstream?

Generally, the contribution feels like a great usability improvement, and looking forward to merge it 😄

Hi @ekatef, Regarding storing the image in pypsa-earth, I think @drifter089 have more idea on how to handle it and make the transmission easy.

@ekatef
Copy link
Member

ekatef commented Dec 23, 2024

Hello @drifter089 @GbotemiB, fantastic work!! My feeling is that we are close to finalise 🚀
Have added some comments which are mainly quite minor. My only major concern related to the overall architecture to manage docker infrastructure. Would it be possible to use pypsa-earth directly to store the image and clone the repo? Obviously, we would need you support for that. May you be willing to take the lead in managing dockerisation directly in the upstream?
Generally, the contribution feels like a great usability improvement, and looking forward to merge it 😄

Hi @ekatef, Regarding storing the image in pypsa-earth, I think @drifter089 have more idea on how to handle it and make the transmission easy.

Thanks a lot for introducing the changes @GbotemiB! 😄 Looks great!

For hosting the image, I think we can use Akshat's repo at first handling docker as an experimental feature and transition to a more stable configuration. For now, ready to merge 😄

@ekatef ekatef merged commit 1a5991b into pypsa-meets-earth:main Dec 23, 2024
6 checks passed
@ekatef
Copy link
Member

ekatef commented Dec 23, 2024

Merged 🎉 We finally have a container distribution 😄
Thanks a lot for the great contribution @GbotemiB @drifter089 !

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.

6 participants