[Core] Pass logs through if sphinx-doctest is running

[peytondmurray]

Why are these changes needed?

logging.dictConfig disables all currently-registered loggers unless you tell it not to. This had the effect of suppressing all sphinx log messages. logging.dictConfig has an option to not do this using the disable_existing_loggers option, which I've configured to avoid this going forward.

Related issue number

Closes #37711.

Checks

• I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
• I've run scripts/format.sh to lint the changes in this PR.
• I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've added any new APIs to the API Reference. For example, if I added a
    method in Tune, I've added it in doc/source/tune/api/ under the
    corresponding .rst file.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested :(

[peytondmurray]

For completeness, I've added an image of the output from building the docs with this branch:

And here's for the current master branch:

And here's for the current master branch but without the SKIP_LOG_RESET=True build flag:

@maxpumperla So I think this shows the issue is fixed. Is there anything else that needs to be done to merge this?

[maxpumperla]

@peytondmurray looks good from the docs side of things, but can't judge the impact of this on the rest of Ray. I trust you ensure the failing 17 CI tests are unrelated.

[peytondmurray]

@maxpumperla It's hard to say, the flaky test tracker shows a lot of broken test suites in the past and recently. I don't have much confidence in the CI:

I'll try rebasing on master to see if any of these have been fixed, but judging by flaky tracker it looks like a lot of doc tests are broken right now.

@rkooo567 I'd appreciate a second look at this.

[peytondmurray]

@rkooo567 Still waiting on review here. I'll try rebasing again to see if the bazel issues have been sorted out on master.

[rkooo567]

Can you add a test case from #37711?

[peytondmurray]

Done; tested that it fails on master as expected, but passes on this branch.

[rickyyx]

Is it expected the readthedocs build fails?

[peytondmurray]

I'm not sure, where can I check the build log for that? I don't see it in the list of failures:

• Doctest looks unrelated
  
• Lint is on a file unaffected by this PR, so must be failing on master:
  
• Python 3.7 legacy dependency ML failed to download a dependency
• Small external redis tests are flaky:
  
• No idea about the failing windows test:
  

[rickyyx]

This is the failure I was referring to: https://readthedocs.com/projects/anyscale-ray/builds/1618200/

Seems this happens on readthedocs. cc @maxpumperla any ideas?

[peytondmurray]

From the log:

build finished with problems, 2 warnings.

There are two sets of warnings, sphinx's output doesn't tell you which are really responsible for the message above. During the reading phase:

/home/docs/checkouts/readthedocs.org/user_builds/anyscale-ray/envs/36306/lib/python3.9/site-packages/nbformat/__init__.py:93: MissingIDFieldWarning: Code cell is missing an id field, this will become a hard error in future nbformat versions. You may want to use `normalize()` on your notebooks before validations (available since nbformat 5.1.4). Previous versions of nbformat are fixing this issue transparently, and will stop doing so in the future.
  validate(nb)

...

/home/docs/checkouts/readthedocs.org/user_builds/anyscale-ray/envs/36306/lib/python3.9/site-packages/nbformat/__init__.py:93: MissingIDFieldWarning: Code cell is missing an id field, this will become a hard error in future nbformat versions. You may want to use `normalize()` on your notebooks before validations (available since nbformat 5.1.4). Previous versions of nbformat are fixing this issue transparently, and will stop doing so in the future.
  validate(nb)

...

2023-07-24 23:24:41,679	WARNING deprecation.py:50 -- DeprecationWarning: `build_tf_policy` has been deprecated. This will raise an error in the future!
2023-07-24 23:24:41,689	WARNING deprecation.py:50 -- DeprecationWarning: `build_policy_class` has been deprecated. This will raise an error in the future!

And during the writing phase:

/home/docs/checkouts/readthedocs.org/user_builds/anyscale-ray/envs/36306/lib/python3.9/site-packages/tune_sklearn/tune_gridsearch.py:docstring of sklearn.utils._metadata_requests.RequestMethod.__get__.<locals>.func:3: WARNING: undefined label: metadata_routing
/home/docs/checkouts/readthedocs.org/user_builds/anyscale-ray/envs/36306/lib/python3.9/site-packages/tune_sklearn/tune_search.py:docstring of sklearn.utils._metadata_requests.RequestMethod.__get__.<locals>.func:3: WARNING: undefined label: metadata_routing

I'm guessing it's the two in the writing phase that have undefined labels that are the offending warnings, though again, sphinx is unclear here. It seems like the build completes successfully, but still returns a nonzero exit code if there are any warnings. Seems like the docs for tune_search and tune_gridsearch have bad sphinx references.

[rickyyx]

Looks like recent PRs merged to the master don't have readthedocs failure - could we merge with master to doublecheck?

[peytondmurray]

Locally, at least, the build finishes with no problems:

[rickyyx]

Gotcha - waiting for @rkooo567 's review.

[rickyyx]

The read the docs on the master, however, seems to be passing https://readthedocs.org/projects/ray/builds/ ?

[peytondmurray]

I'm not sure about this - maybe @rkooo567 or @maxpumperla can comment. I didn't make any changes to the documentation here, so the fact that there are undefined references in the RTD build is weird, though the RTD build system seems to be doing some stuff I didn't expect:

There a bunch of extra pip install <dependency> lines here :/. I think all that should be needed is pip install ./python && pip install -r ./docs/requirements_docs.txt. If there are additional build dependencies that are needed, we should add them to the required dependencies, right? There might also be something about building on Python 3.9 that doesn't work. I'll explore different versions to see if I can replicate the build environment.

[peytondmurray]

Okay, after a little investigation it seems like I'm not really able to replicate the RTD build environment based on the log output I see here. I tried creating a python 3.9.17 virtualenv, then doing

pip install -U pip setuptools
pip install --upgrade --no-cache-dir pillow mock==1.0.1 'alabaster>=0.7,<0.8,!=0.7.5' commonmark==0.9.1 recommonmark==0.5.0 sphinx sphinx-rtd-theme 'readthedocs-sphinx-ext<2.3'
pip install --exists-action=w --no-cache-dir -r doc/requirements-doc.txt

to install dependencies. The following command fails:

python -m sphinx -T -E -W --keep-going -b html -d _build/doctrees -D language=en . _build/html

because we aren't in ray/doc/source/, where the sphinx conf.py is stored. If we cd to that directory and then run the command, it also fails because ray itself is not installed. I'm guessing that the asdf python environment already contains ray installed, but I haven't double checked this.

In any case, going back to ray/python and doing pip install -ve .[all] installs ray, but breaks the myst_nb extension due to incompatibilities between the doc requirements and the ray requirements:

Running Sphinx v4.3.2
2023-07-26 15:58:01,325	WARNING __init__.py:21 -- Package pickle5 becomes unnecessary in Python 3.8 and above. Its presence may confuse libraries including Ray. Please uninstall the package.
loading translations [en]... done

Traceback (most recent call last):
  File "/home/pdmurray/.pyenv/versions/ray3917/lib/python3.9/site-packages/sphinx/registry.py", line 429, in load_extension
    mod = import_module(extname)
  File "/home/pdmurray/.pyenv/versions/3.9.17/lib/python3.9/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1030, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1007, in _find_and_load
  File "<frozen importlib._bootstrap>", line 986, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 680, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 850, in exec_module
  File "<frozen importlib._bootstrap>", line 228, in _call_with_frames_removed
  File "/home/pdmurray/.pyenv/versions/ray3917/lib/python3.9/site-packages/myst_nb/__init__.py", line 35, in <module>
    from .parser import NotebookParser
  File "/home/pdmurray/.pyenv/versions/ray3917/lib/python3.9/site-packages/myst_nb/parser.py", line 13, in <module>
    from myst_parser.sphinx_parser import MystParser
  File "/home/pdmurray/.pyenv/versions/ray3917/lib/python3.9/site-packages/myst_parser/sphinx_parser.py", line 9, in <module>
    from markdown_it.utils import AttrDict
ImportError: cannot import name 'AttrDict' from 'markdown_it.utils' (/home/pdmurray/.pyenv/versions/ray3917/lib/python3.9/site-packages/markdown_it/utils.py)

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/pdmurray/.pyenv/versions/ray3917/lib/python3.9/site-packages/sphinx/cmd/build.py", line 276, in build_main
    app = Sphinx(args.sourcedir, args.confdir, args.outputdir,
  File "/home/pdmurray/.pyenv/versions/ray3917/lib/python3.9/site-packages/sphinx/application.py", line 237, in __init__
    self.setup_extension(extension)
  File "/home/pdmurray/.pyenv/versions/ray3917/lib/python3.9/site-packages/sphinx/application.py", line 394, in setup_extension
    self.registry.load_extension(self, extname)
  File "/home/pdmurray/.pyenv/versions/ray3917/lib/python3.9/site-packages/sphinx/registry.py", line 432, in load_extension
    raise ExtensionError(__('Could not import extension %s') % extname,
sphinx.errors.ExtensionError: Could not import extension myst_nb (exception: cannot import name 'AttrDict' from 'markdown_it.utils' (/home/pdmurray/.pyenv/versions/ray3917/lib/python3.9/site-packages/markdown_it/utils.py))

Extension error:
Could not import extension myst_nb (exception: cannot import name 'AttrDict' from 'markdown_it.utils' (/home/pdmurray/.pyenv/versions/ray3917/lib/python3.9/site-packages/markdown_it/utils.py))

So instead we need to go to ray/doc and do pip install -r requirements_doc.txt to reinstall the correct doc dependencies. Finally, heading back to ray/doc/source/ and running

python -m sphinx -T -E -W --keep-going -b html -d _build/doctrees -D language=en . _build/html

once more will start the doc build. Here's the result (image because it's easier):

6 warnings instead of 2! I'm going to try

1. Just running make develop like I usually do when developing to keep the build parameters the same.
2. Switch to another python version in a fresh virtualenv to test the same procedure.

[peytondmurray]

Reporting back about (1) ☝️ above:

I ran make clean and make develop inside ray/doc/. The build succeeded with 84 warnings (might be hard to see):

Not really sure why, here - it's the same build environment. I guess the Makefile is calling sphinx-build rather than sphinx directly 🤔. I'll try with 3.11, as 3.10 seems to work just fine locally.

[maxpumperla]

@peytondmurray now that we have an rtd yaml https://github.com/ray-project/ray/blob/master/.readthedocs.yaml, can't we control the build command to do what we want?

[rickyyx]

Thanks for all the detailed investigation!

I have a question with this change: wo we will always generate logging config - which we were not? Is this change causing the build to fail?

[peytondmurray]

Yes, we will always generate logging config, which is what we want. The original problem we were addressing here is that the sphinx build loggers get disabled if you don't have 'disable_existing_loggers': False in the call to dictConfig to set up the loggers. I don't think that's the issue here.

RTD's fail-on-warning was only enabled last week; I think that's relevant here.

[peytondmurray]

@maxpumperla @rickyyx Any idea how the RTD build environment is set up? Looking at the .readthedocs.yml I don't see anywhere that ray itself is being installed. For me this causes problems when building the docs locally.

One other thing I noticed: the makefile generated by sphinx-quickstart doesn't actually clean out all the build artifacts:

After doing git clean -fdx ./ to actually remove everything, I was able to replicate the build issue with 2 warnings locally. Here's the same two failing labels:

[peytondmurray]

Okay, after some more reading it looks like this is related to a recent change in sklearn 1.3.0 that added a new public method for metadata routing. Since the change in the dependency, estimators that inherit from sklearn.base.BaseEstimator are now trying to render documentation for this method. Normally this would be fine, but the docstring for the BaseEstimator contains a reference to another part of the sklearn documentation, which obviously isn't a part of the ray docs. This thorough explanation provided the background.

As to why this is only showing up now: I still don't know how the RTD build environment is constructed, so I suspect that whatever environment was being used on master is using sklearn<1.3.0 or something like that. Not really sure why the build environment in this PR would be different though.

The fix for this is to enable sphinx.ext.intersphinx to allow linking to sklearn documentation - see microsoft/LightGBM#5956. With this, we fix warnings and make the documentation easier for users to use by giving them direct links to sklearn docs.

[peytondmurray]

Not really sure if the other stuff should be failing, but there's a lot of red on https://flaky-tests.ray.io/. From what I can see the failures unrelated. RTD now works.

[rickyyx]

The fix for this is to enable sphinx.ext.intersphinx to allow linking to sklearn documentation - see microsoft/LightGBM#5956. With this, we fix warnings and make the documentation easier for users to use by giving them direct links to sklearn docs.

Really great investigation @peytondmurray! Great catch and thanks for digging into this.

Can you merge with master (which is in a better shape now I believe).

[rickyyx]

Thanks again @peytondmurray for the great work on iterating this PR - the comments and investigation was amazing.

Merging since failed tests are flaky in master