Migrate from tox 3.28 to nox

[arjxn-py]

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

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

Key checklist:

• No style issues: $ pre-commit run (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
• The documentation builds: $ python run-tests.py --doctest

You can run unit and doctests together at once, using $ python run-tests.py --quick.

Further checks:

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

[codecov]

Codecov Report

Patch and project coverage have no change.

Comparison is base (bf85811) 99.71% compared to head (d526dfc) 99.71%.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3005   +/-   ##
========================================
  Coverage    99.71%   99.71%           
========================================
  Files          248      248           
  Lines        18738    18738           
========================================
  Hits         18684    18684           
  Misses          54       54           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[agriyakhetarpal]

There was also a discussion on switching from Tox to Nox in #2537 (comment), something that I mentioned in my proposal. Though we can also take a look at wntrblm/nox#687 after it gets merged to do that

[arjxn-py]

There was also a discussion on switching from Tox to Nox in #2537 (comment), something that I mentioned in my proposal. Though we can also take a look at wntrblm/nox#687 after it gets merged to do that

Yes @agriyakhetarpal, nox is immediate next to give try to but before that I am trying to make use of tox4, the resource you've mentioned is really helpful. I'd be really glad to discuss this issue & have your suggestions & feedbacks further.
Lately I was geting these logs constantly even after making major changes in tox.ini but now I'm declaring each environment explicitely one-by-one (maybe which will increase the load on CI).

[agriyakhetarpal]

For the test suite, I was also looking at this library called test-stages – it allows us to group particular sections of Tox and Nox tests using parse-stages and run those groups in parallel, and seems to be maintained too.

I was trying to get this set up locally some time ago but I could not get anything concrete so as to push changes. Maybe you can look into this, we might be able to bring CI time down on Linux as well with this by running tox -e docs in conjunction with tox -e unit or, say, the trio of examples + unit + integration together since the example tests take the longest amount of time

[arjxn-py]

For the test suite, I was also looking at this library called test-stages – it allows us to group particular sections of Tox and Nox tests using parse-stages and run those groups in parallel, and seems to be maintained too.

Apologies for the delay in response @agriyakhetarpal, will surely give it a try. Thankfully, for now almost everything has been migrated to nox with trivial differences in the completion time. I am also attaching logs for further reference.

[agriyakhetarpal]

Though I cannot review or approve the PR, looks good to me! You also probably need to rewrite the documentation to change all instances of tox to nox and update the instructions to account for differences in how to run the tests and for installation from source

[Saransh-cpp]

Taking a quick look, this looks amazing, and the CI is passing too. I'll review this in a few days!

[Saransh-cpp]

Amazing work, thanks, @arjxn-py! Some quick comments below. Could you also completely remove the tox.ini file and check if the workflows still pass? I'll have another look on this in a while and try running the testing suite locally with nox.

[arjxn-py]

@Saransh-cpp here are the scheduled test logs after the latest changes.
Coverage session is failing while executing coverage combine with the same response - No data to combine locally as well.

[agriyakhetarpal]

coverage.py reads these parameters from configuration files; such as setup.cfg, or tox.ini through the [coverage:run] section header (coverage docs). But it doesn't read from noxfile.py yet (I guess). So we need to create a .coveragerc configuration file in the root folder with these options:

[run]
source = pybamm
concurrency = multiprocessing

or one may pass it as a command-line argument via --concurrency=multiprocessing. Apparently, we can't pass --source as an argument, so a new configuration file should be the way to go. These parameters were not found when tox.ini was deleted

[arjxn-py]

Thanks @agriyakhetarpal, really so nice of you.

[Saransh-cpp]

Thanks, @arjxn-py and @agriyakhetarpal! Looks much better! Some comments below. Also, @arjxn-py, you might have to merge the develop branch to get all the changes Agriya has recently made into your PR.

[agriyakhetarpal]

Also, we still need some more nox sessions in noxfile.py for usage in developer workflows locally:

1. dev / nox -s dev (to replicate tox -e dev, which installs PyBaMM and all of its extra dependencies locally for development purposes)
2. docs / nox -s docs (to replicate tox -e docs which serves the documentation locally with sphinx-autobuild)
3. tests / nox -s tests (to replicate tox -e tests, which wraps unit tests and integration tests with one command)

[arjxn-py]

Also, we still need some more nox sessions in noxfile.py for usage in developer workflows locally.

Yes @agriyakhetarpal I am still making those sessions.

[agriyakhetarpal]

I have plans to open a PR to cache these environments, you may add that too to the list

[agriyakhetarpal]

I ran this locally on macOS; all commands worked as expected, except for a few:

1. It would be great if there were a way to make nox -s dev run like nox -s dev --verbose by default, otherwise, just the message nox > Session dev was successful. is displayed and no package installation logs are shown in the command line

2. Running the unit tests with nox -s unit skipped 50 tests: 27 because scikits.odes was not installed and 23 because jax was not installed. Should we install them on macOS too? (looks like they were skipped in CI as well)

3. The nox -e docs command was not successful because sphinx-autobuild was not installed (which is weird since it worked fine when I tested it earlier). We should add it in the docs extras in setup.py

4. If nox -s pybamm-requires is executed again, it gives another error: OSError: [Errno 66] Directory not empty: './pybind11'. We can do this, rather:

if not os.path.exists("./pybind11"):
    session.run("git", "clone", "https://github.com/pybind/pybind11.git", "pybind11/", external=True,)

6. L13 in noxfile.py

if sys.platform != "win32" or sys.platform != "darwin":

Here, or sys.platform != "darwin" is redundant—because we want nox -s pybamm-requires to not run on just Windows, and to run on both other platforms. We can just use either if sys.platform != "win32" or use if sys.platform == "darwin" or sys.platform == "linux" (both are the same). Another reason why this is currently redundant is that it has a logical error; an or Boolean operator rather than an and operator – if in any case we had to exclude macOS here, we would have had to use and (De Morgan's laws 😄). For Windows users, we could throw an error too instead, notifying them that the command is available on Linux and macOS only. This way nox will avert creating a futile environment altogether

[arjxn-py]

Thanks @Saransh-cpp @agriyakhetarpal for the detailed review, I'll look into the suggested things.

[arjxn-py]

Running the unit tests with nox -s unit skipped 50 tests: 27 because scikits.odes was not installed and 23 because jax was not installed. Should we install them on macOS too? (looks like they were skipped in CI as well)

As far as I remember we were doing the same with tox as well but if you people would like scikits.odes & jax on macos too, I can look into it.

[agriyakhetarpal]

I see, on a fresh PyBaMM installation on macOS with tox it skips 1 test for me though (it does install both scikits.odes and jax using pybamm_install_jax)

[arjxn-py]

I see, on a fresh PyBaMM installation on macOS with tox it skips 1 test for me though (it does install both scikits.odes and jax using pybamm_install_jax)

I'm not sure how it is installing scikits.odes with pybamm_install_jax can you once try testing again after removing unit folder from .tox/ directory.

[agriyakhetarpal]

I double-checked to be sure, yes... and I always delete the .tox/ directory. Will test once again and edit this comment.

[Saransh-cpp]

Running the unit tests with nox -s unit skipped 50 tests: 27 because scikits.odes was not installed and 23 because jax was not installed. Should we install them on macOS too? (looks like they were skipped in CI as well)

We discussed this, but to make this PR concise (it already has 100+ comments haha), we decided to just mirror the tox file at the moment. Everything else can be discussed and added iteratively.

[arjxn-py]

My bad, sorry both.
I fixed it for nightly tests (i.e. Ubuntu) please give me a couple of hours to look into this too.

[arjxn-py]

Now that I am installing scikits.odes on macos, the CI is failing.
I still am wondering that we are doing the same with tox.ini as well (not installing scikits.odes on windows & macos) :

unit-!windows-!mac: sh -c "pybamm_install_jax"
!windows-!mac: scikits.odes

These tests are getting skipped with tox as well on macos here
Please correct me if i'm wrong.

[Saransh-cpp]

I think you misread my comment @arjxn-py. We can skip installing scikits.odes and jax on macos for now.

[Saransh-cpp]

The nox -e docs command was not successful because sphinx-autobuild was not installed (which is weird since it worked fine when I tested it earlier). We should add it in the docs extras in setup.py

For Windows users, we could throw an error too instead, notifying them that the command is available on Linux and macOS only. This way nox will avert creating a futile environment altogether

@arjxn-py were these review points resolved?

[arjxn-py]

Yes @Saransh-cpp resolved.

[Saransh-cpp]

Looks perfect now, thanks @arjxn-py 🙂

Will merge this tonight if there are no blockers