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

Add and update dependencies for Docker images #3317

Merged
merged 10 commits into from
Nov 17, 2023

Conversation

agriyakhetarpal
Copy link
Member

Description

See #3312

  1. Bumps Python version to 3.11
  2. Removes dependence on CMake==3.22 and the latest stable CMake version is used
  3. nox is installed by default in both the base and the pybamm environments
  4. Includes all extras dependencies and [docs] dependencies, adds pandoc, and and a TeXLive distribution (for the latexify tests, rendering notebooks in the built documentation, et cetera).

Note

This PR should be merged after #3316

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
  • All tests pass: $ python run-tests.py --all (or $ nox -s tests)
  • The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@codecov
Copy link

codecov bot commented Sep 10, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b917acf) 99.58% compared to head (172b1f4) 99.58%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3317   +/-   ##
========================================
  Coverage    99.58%   99.58%           
========================================
  Files          256      256           
  Lines        20117    20117           
========================================
  Hits         20034    20034           
  Misses          83       83           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

scripts/Dockerfile Outdated Show resolved Hide resolved
scripts/Dockerfile Outdated Show resolved Hide resolved
Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Thanks, @agriyakhetarpal! A few comments below -

scripts/Dockerfile Outdated Show resolved Hide resolved
scripts/Dockerfile Outdated Show resolved Hide resolved
scripts/Dockerfile Outdated Show resolved Hide resolved
@agriyakhetarpal
Copy link
Member Author

@Saransh-cpp and I both have ARM machines so debugging might be a bit difficult, @arjxn-py could you test the new changes locally?

@arjxn-py
Copy link
Member

arjxn-py commented Oct 10, 2023

could you test the new changes locally?

Hey, so sorry for the delay in response, I'll test this and confirm.

@agriyakhetarpal
Copy link
Member Author

Hey, so sorry for the delay in response, I'll test this and confirm.

No worries, I tested the images locally and fixed some errors with CMake. It requires a non-user installation if one installs it with pip otherwise the system CMake is used for compilation (which could be outdated). We still need the Jax versions to be bumped for those sets of images (JAX and ALL) to work natively on aarch64 (#3443), but the other images (IDAKLU and ODES) seem to be working well since all solver-related tests pass.

@agriyakhetarpal
Copy link
Member Author

This should be ready to merge but only after #3430

@agriyakhetarpal
Copy link
Member Author

A gentle bump here, @arjxn-py and @Saransh-cpp! The Docker images here were working for me locally – now that we have multi-architecture images being pushed, I think it should be fine to go ahead with this.

Copy link
Member

@arjxn-py arjxn-py left a comment

Choose a reason for hiding this comment

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

Changes looks nice, thanks @agriyakhetarpal.
I'll also confirm once after testing Dockerfile locally.

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Looks good, sorry for the delay. Thanks, @agriyakhetarpal!

@Saransh-cpp Saransh-cpp merged commit 1a38b25 into pybamm-team:develop Nov 17, 2023
23 of 35 checks passed
@agriyakhetarpal agriyakhetarpal deleted the docker-dependencies branch November 17, 2023 18:33
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.

3 participants