PyBaMM icon indicating copy to clipboard operation
PyBaMM copied to clipboard

Removed Google colab error warning.

Open prady0t opened this issue 11 months ago • 11 comments

Description

Follow-up of #3740

Fixes # (issue)

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)
  • [x] Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • [x] 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)
  • [x] 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

prady0t avatar Mar 11 '24 20:03 prady0t

Thanks @prady0t, could you remove these lines:

# Temporary fix for Python 3.12 CI. TODO: remove after
# https://bitbucket.org/pybtex-devs/pybtex/issues/169/replace-pkg_resources-with
# is fixed
session.install("setuptools", silent=False)

wherever they exist in noxfile.py?

agriyakhetarpal avatar Mar 11 '24 20:03 agriyakhetarpal

Sorry for making changes lazily. 🙂 I've updated the PR.

prady0t avatar Mar 11 '24 20:03 prady0t

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.60%. Comparing base (8056d22) to head (abc7cc6).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3886      +/-   ##
===========================================
- Coverage    99.60%   99.60%   -0.01%     
===========================================
  Files          259      259              
  Lines        21273    21268       -5     
===========================================
- Hits         21189    21184       -5     
  Misses          84       84              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 11 '24 20:03 codecov[bot]

Also, it looks like the doctests on Python 3.12 are failing (strange) but all other jobs on Python 3.12 pass. Could we re-add the setuptools installation to the doctests session for now?

agriyakhetarpal avatar Mar 12 '24 05:03 agriyakhetarpal

Also, it looks like the doctests on Python 3.12 are failing (strange) but all other jobs on Python 3.12 pass. Could we re-add the setuptools installation to the doctests session for now?

It could be a latex parsing issue: SyntaxWarning: invalid escape sequence '\m' is caused by self.spatial_unit = "$\mu$m" Maybe we need to make them raw strings or something:

self.spatial_unit = r"$\mu$m"

kratman avatar Mar 12 '24 17:03 kratman

Yes, that warning would be good to fix too. But the failure seems to be coming from a Sphinx-related warning: sphinxcontrib_bibtex can't find pkg_resources – even though it is enabled with setuptools as a seed package in other sessions 😕

agriyakhetarpal avatar Mar 12 '24 17:03 agriyakhetarpal

I've re added setuptools installation to the doctests session and removed checks for error messages.

prady0t avatar Mar 13 '24 05:03 prady0t

The doctests and example scripts are still failing on Python 3.12, and I'm not sure why. We have two options:

  1. Add the session.install("setuptools") hack back again for the time being
  2. Modify these sessions in the workflows to run on Python 3.11 instead.

I would go forward with the first option at this time, because using the latest available Python version will help us catch any related (or unrelated) issues early.

Not all of our functionality works on 3.12, so 3.11 is probably better

kratman avatar Mar 13 '24 17:03 kratman

Not all of our functionality works on 3.12, so 3.11 is probably better

Yes, I had considered that when posting my comment – I had checked the example scripts and none of them are currently using unsupported functionality (which is scikits.odes for Python 3.12). So Python 3.12 would be fine (for the docs-related tests, it also helps catch warnings/errors early because most documentation dependencies are pure-Python and do not have an upper bound on their Python version requirement). So we can go ahead with either of those, but I tend to lean slightly on 3.12. Both Python versions have a long time till their EOL.

agriyakhetarpal avatar Mar 13 '24 17:03 agriyakhetarpal

I tend to lean slightly on 3.12. Both Python versions have a long time till their EOL.

I would say that it is better to go with a version where everything runs, we can always upgrade the version later. It is for the documentation generation, not something that must be the newest python

kratman avatar Mar 13 '24 18:03 kratman

I would say that it is better to go with a version where everything runs, we can always upgrade the version later. It is for the documentation generation, not something that must be the newest python

Sure, let's do it for just the doctests, but keep Python 3.12 for the example scripts (because it matches with the example notebooks tests).

agriyakhetarpal avatar Mar 13 '24 18:03 agriyakhetarpal

would doing

@nox.session(name="doctests", python="3.11")

work?

prady0t avatar Mar 17 '24 18:03 prady0t

would doing

@nox.session(name="doctests", python="3.11")

work?

You don't need to change the nox configuration for the doctests. We just need to fix the CI and we should be good to go – just alter test_on_push.yml to use Python 3.11 in the doctests job instead of Python 3.12. The only change required in noxfile.py is to bring back the session.install("setuptools") line that you had removed from the example scripts session.

agriyakhetarpal avatar Mar 17 '24 21:03 agriyakhetarpal

Yess. I'll be more than happy to rework here.

prady0t avatar Mar 18 '24 12:03 prady0t