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

Reduce the number of Docker Images to just one image #3666

Closed
arjxn-py opened this issue Dec 26, 2023 · 7 comments · Fixed by #3992
Closed

Reduce the number of Docker Images to just one image #3666

arjxn-py opened this issue Dec 26, 2023 · 7 comments · Fixed by #3992
Labels
difficulty: easy A good issue for someone new. Can be done in a few hours priority: medium To be resolved if time allows

Comments

@arjxn-py
Copy link
Member

arjxn-py commented Dec 26, 2023

Currently we build and push 5 variants of Docker Images regularly i.e. latest, jax, odes, idaklu & all.
The idea behind this issue is to have a single standard image variant to be updated regularly to save resources and make things simple for developers.
As discussed in #3661

@arjxn-py arjxn-py added the needs-reply Needs further information from the author and may be closed if no response is received label Dec 26, 2023
@agriyakhetarpal agriyakhetarpal changed the title Reduce the number of Docker Images Discussion: reduce the number of Docker Images? Dec 26, 2023
@agriyakhetarpal agriyakhetarpal removed the needs-reply Needs further information from the author and may be closed if no response is received label Dec 26, 2023
@agriyakhetarpal
Copy link
Member

Removed the needs-reply label lest the issue gets closed automatically after thirty days. I renamed the title instead to explicitly mark it as a discussion

@agriyakhetarpal
Copy link
Member

Prior to starting with #3692, I think we should arrive at a consensus here so as to reduce coordinated effort.


Reiterating from #3661 for more visibility, the current reasoning for reducing the number of Docker images is as follows:

  1. The pybamm/pybamm:latest image, i.e., without any solvers is just the same as cloning the GitHub repository or running pip install git+https://github.com/pybamm-team/pybamm, since it does not require any compiled steps. The procedure of installing PyBaMM without pybind11 present for linkage is essentially going to skip the IDAKLU extension, without which PyBaMM is a pure-Python package.
  2. The pybamm/pybamm:jax image just installs the JAX-based solvers, whose installation is very fast owing to the presence of wheels on PyPI. There is just one command extra for this purpose, which is just pip install pybamm[jax] – and with Idaklu-Jax Interface #3658, it might make sense to provide the IDAKLU solver and the JAX solver together instead of in separate images.
  3. pybamm/pybamm:odes (i.e., the ODES argument) re-uses the SUNDIALS installation script that we use for the compilation of the IDAKLU solver and just points SUNDIALS_INST to the required path.
  4. In a nutshell, the IDAKLU image is the most important one to target and maintain, and IMO the only one we should maintain. We can explore reducing the build-time for the Docker image through the use of aggressive build caching (possible for tools like poetry, will have to explore other methods) and make the ALL image the default one, which would then contain the IDAKLU, scikits.odes (re-uses the SUNDIALS compiled for the IDAKLU solver), and the JAX solvers.

This should simplify the tags for the image too – we just have to support pybamm/pybamm:latest (and pybamm/pybamm:vYY.M after #3661 gets completed).

@agriyakhetarpal
Copy link
Member

It was decided in the last developer meeting on 19th February that we can indeed reduce the number of Docker images, and establish just one image to contain all three solvers (scikits.odes, JaxSolver, and IDAKLU) and remove the image without the solvers. This can be taken up with #3692, I can revisit it this week and then get #3661 unblocked.

@agriyakhetarpal agriyakhetarpal changed the title Discussion: reduce the number of Docker Images? Reduce the number of Docker Images to just one image Feb 23, 2024
@agriyakhetarpal agriyakhetarpal added difficulty: easy A good issue for someone new. Can be done in a few hours priority: medium To be resolved if time allows labels Feb 23, 2024
@agriyakhetarpal
Copy link
Member

I can't assign @santacodes to this issue apparently, but this is being worked on along with #3692

@agriyakhetarpal
Copy link
Member

I think #3879 is going to take a bit of time for now, so we can close this issue at least (since ODES has been removed in #3932). Could you do this separately, @santacodes, and rebase #3901 afterwards?

@santacodes
Copy link
Contributor

santacodes commented Apr 10, 2024

I think #3879 is going to take a bit of time for now, so we can close this issue at least (since ODES has been removed in #3932). Could you do this separately, @santacodes, and rebase #3901 afterwards?

Sure that would work, I think it would be then safe to close this issue

@santacodes
Copy link
Contributor

Working on a PR, with some changes in dockerfile, docker workflow and docs. Hopefully will open it by tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: easy A good issue for someone new. Can be done in a few hours priority: medium To be resolved if time allows
Projects
None yet
3 participants