Fix Transforms cells and update contributors guide

[IamShubhamGupto]

• Closes [BUG] broken cells in transform page of docs  #573
• Tests added
• Passes pre-commit run --all-files
• User visible changes (including notable bug fixes) are documented in whats-new.rst
• New functions/methods are listed in api.rst
• Updated doc/environment.yml to reflect dependencies of doc/transform.ipynb
• Updated doc/contributor_guide.rst to show how to setup RTD environment

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[IamShubhamGupto]

strange conda installed sphinx v7.2.6 but compiled using v4.5.0, im going to add hard version numbers to see if that helps

[IamShubhamGupto]

I can't seem to be able to reproduce these errors on my setup, not sure what im missing. boundary_condition.rst compiles fine with the same setup

[IamShubhamGupto]

@jbusecke take a look when possible, the additional commits are just me trying to fix the warnings individually per rst file. I can't reproduce those even with the same flags on my laptop + couldn't figure how to tell sphinx to ignore warnings when building.

Would be nice to edit the action that builds RTDs

But this should build up to the #623 issue

[jbusecke]

So from the earlier builds it seems there is an option to specify :okwarning: in the block itself?

Warning in /home/docs/checkouts/readthedocs.org/user_builds/xgcm/checkouts/627/doc/ufunc_examples.rst at block ending on line 87
Specify :okwarning: as an option in the ipython:: block to suppress this message

Did you try that yet?

[jbusecke]

I just started #628 which will actually remove that warning being called in the first place! So for that particular issue it might be cleanest solution.

[IamShubhamGupto]

So from the earlier builds it seems there is an option to specify :okwarning: in the block itself?

Warning in /home/docs/checkouts/readthedocs.org/user_builds/xgcm/checkouts/627/doc/ufunc_examples.rst at block ending on line 87
Specify :okwarning: as an option in the ipython:: block to suppress this message

Did you try that yet?

Yep you can see the build here - https://readthedocs.org/projects/xgcm/builds/24038034/ -seems more angry

I tried looking up documentation https://ipython.readthedocs.io/en/stable/sphinxext.html but not sure why my implementation didn't help

[IamShubhamGupto]

I just started #628 which will actually remove that warning being called in the first place! So for that particular issue it might be cleanest solution.

best would be to modify the action call
python -m sphinx -T -b html -d _build/doctrees -D language=en . $READTHEDOCS_OUTPUT/html if we can add a --keep-going flag it will compile regardless of warnings and errors

[jbusecke]

If the warning eliminated by #628 is the only one that upset the docs build, we should just move on with it failing on warnings? I think this provides a nice warning system for us.

[IamShubhamGupto]

If the warning eliminated by #628 is the only one that upset the docs build, we should just move on with it failing on warnings? I think this provides a nice warning system for us.

#628 did seem to eliminate the issue but introduces the problem where the sphinx module installed is not compatible with other modules. I address this in 4933b2a which hardcodes the versions of sphinx to be used.

The above hard codes could cause problems with future version releases. A solution for that would be to hardcode the latest stable versions for all modules and then repeat every couple months or years

[jbusecke]

where the sphinx module installed is not compatible with other modules

Is this locally or during the RTD build?

[IamShubhamGupto]

where the sphinx module installed is not compatible with other modules

Is this locally or during the RTD build?

locally the environment file without hardcodes installs perfectly. I could delete env and verify if required

on RTD build without hardcoded versions, it installs incorrect module versions as seen in #628 build

[jbusecke]

#631 should be automerged soon. If you could refactor this PR on top of main once that happens that would be phenomenal. Sorry for this messy starting point.

[IamShubhamGupto]

not sure why it broke again, its supposed to be easy fix. ill see what happened

[IamShubhamGupto]

@jbusecke this should be good to go, not sure why but I had to move numba to pip dependencies and only then the xgcm import worked.

[jbusecke]

Thanks for working on this. I think we are close. But lets try to understand why we cannot install numba via conda. That would be cleaner IMO (see my review comments).

[jbusecke]

I would like to avoid having to resort to this sort of fix here. Lets definitely keep anything related to the warnings out of the user facing (i.e. what you see on the on the built doc website).

[jbusecke]

Why are you using anaconda and defaults here? In our CI builds conda-forge is sufficient.

We use this numba channel for upstream builds, but I hope this is not necessary here

[jbusecke]

I think lllvmlite is a dependency of numba and should be automatically pulled when installing numba (via conda at least)

[IamShubhamGupto]

I think lllvmlite is a dependency of numba and should be automatically pulled when installing numba (via conda at least)

That's true and it has been happening implicitly for all the previous successful builds. Ill give it a try again without llvmlite now that ive added conda-forge

[jbusecke]

We do not need pre-commit for the docs, do we?

[IamShubhamGupto]

We do not need pre-commit for the docs, do we?

The pre-commit bot runs on all PRs regardless of the changes. I think we should promote users fixing their formatting issues rather than waiting for the bot to try and fail

[jbusecke]

The pre-commit bot runs on all PRs regardless of the changes.

It only does once you have installed it! But I think you are making a great point about fixing formatting. But lets parse the two different scenarios here:

• What is needed to exclusively build the docs
• What is needed to locally develop the docs

I think generally these should be separate. If we could live with a pure pip install we could cleanly define each set of requirements like here, but due to the pandoc and numba dependency we are somewhat hampered to use conda here.

Can we leave this env file as is and instruct the user to install the cloned repo with `pip install ".[dev]" (where we would add the pre-commit dependency)?

Also are we sure that all the docs are actually checked by the pre-commit hooks?

Writing this I realize that this should probably be separated out into another PR to keep this already large PR more focused and keep it moving? We can continue the discussion over there.

[IamShubhamGupto]

It only does once you have installed it! But I think you are making a great point about fixing formatting. But lets parse the two different scenarios here:

• What is needed to exclusively build the docs
• What is needed to locally develop the docs

Since the docs has more than just .rst files - ipynb notebooks as well, the local development and build would be the same? or maybe im missing the point of splitting build vs deployment

Can we leave this env file as is and instruct the user to install the cloned repo with `pip install ".[dev]" (where we would add the pre-commit dependency)?

In order to do that, we need all the dependencies needed to compile docs notebooks to be available when we do pip install .[dev]. I can update that, if thats how we want to do build vs deployment. Then we can remove pre-commit from doc/environment.yml as well

Also are we sure that all the docs are actually checked by the pre-commit hooks?

changes like trailing-whitespace and end-of-file-fixer is run on all file types. The remaining is code related so some notebooks. And in the future if we add more notebooks, then those also need pre-commit linting

Writing this I realize that this should probably be separated out into another PR to keep this already large PR more focused and keep it moving? We can continue the discussion over there.

When it comes to dependencies, the doc/transforms.ipynb did need to add a few more installations before it started working. Let me know what other aspect you want to split from this PR

[jbusecke]

In order to do that, we need all the dependencies needed to compile docs notebooks to be available when we do pip install .[dev]. I can update that, if thats how we want to do build vs deployment. Then we can remove pre-commit from doc/environment.yml as well

Well that would be ideal, but is difficult due to our non-python dependencies. My primary goal here is to not add anything we do not need for the pure CI doc build! This is a slightly different use case from modifiying the docs locally, for which (in my mind) you need the doc build env + dev tools like pre-commit.

[jbusecke]

Then we can remove pre-commit from doc/environment.yml as well

Yes that would be great!

[jbusecke]

When it comes to dependencies, the doc/transforms.ipynb did need to add a few more installations before it started working. Let me know what other aspect you want to split from this PR

Whatever is needed to get the docs build on RTD to pass is totally fine here.