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

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

Merged
merged 3 commits into from
Jul 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions doc/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,12 @@ clean:
rm -rf ./source/rllib/package_ref/doc*

html:
SKIP_LOG_RESET=True $(SPHINXBUILD) -W --keep-going -b html $(ALLSPHINXOPTS) $(BUILDDIR)/html
$(SPHINXBUILD) -W --keep-going -b html $(ALLSPHINXOPTS) $(BUILDDIR)/html
@echo
@echo "Build finished. The HTML pages are in $(BUILDDIR)/html."

develop:
SKIP_LOG_RESET=True FAST=True $(SPHINXBUILD) -b html $(ALLSPHINXOPTS) $(BUILDDIR)/html
FAST=True $(SPHINXBUILD) -b html $(ALLSPHINXOPTS) $(BUILDDIR)/html
@echo
@echo "Build finished. The HTML pages are in $(BUILDDIR)/html."
@echo "View the documentation by opening a browser and going to $(BUILDDIR)/html/index.html."
Expand Down
4 changes: 4 additions & 0 deletions doc/source/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
"sphinx_tabs.tabs",
"sphinx_remove_toctrees",
"sphinx_design",
"sphinx.ext.intersphinx",
]

# Prune deep toc-trees on demand for smaller html and faster builds.
Expand Down Expand Up @@ -107,6 +108,9 @@
"replacements",
]

intersphinx_mapping = {
"sklearn": ("https://scikit-learn.org/stable/", None),
}

# Cache notebook outputs in _build/.jupyter_cache
# To prevent notebook execution, set this to "off". To force re-execution, set this to "force".
Expand Down
6 changes: 1 addition & 5 deletions python/ray/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,7 @@
import os
import sys

# For cases like docs builds, we want the default logging config.
skip_reset = os.environ.get("SKIP_LOG_RESET", False)
if not skip_reset:
log.generate_logging_config()

log.generate_logging_config()
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

logger = logging.getLogger(__name__)


Expand Down
25 changes: 25 additions & 0 deletions python/ray/_private/log.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,30 @@
import re
from logging.config import dictConfig
import threading
from typing import Union


def _print_loggers():
"""Print a formatted list of loggers and their handlers for debugging."""
loggers = {logging.root.name: logging.root}
loggers.update(dict(sorted(logging.root.manager.loggerDict.items())))
for name, logger in loggers.items():
if isinstance(logger, logging.Logger):
print(f" {name}: disabled={logger.disabled}, propagate={logger.propagate}")
for handler in logger.handlers:
print(f" {handler}")


def clear_logger(logger: Union[str, logging.Logger]):
"""Reset a logger, clearing its handlers and enabling propagation.

Args:
logger: Logger to be cleared
"""
if isinstance(logger, str):
logger = logging.getLogger(logger)
logger.propagate = True
logger.handlers.clear()


class ContextFilter(logging.Filter):
Expand Down Expand Up @@ -134,5 +158,6 @@ def generate_logging_config():
"filters": filters,
"handlers": handlers,
"loggers": loggers,
"disable_existing_loggers": False,
}
)
4 changes: 1 addition & 3 deletions python/ray/_private/ray_logging.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,7 @@ def setup_component_logger(
Returns:
the created or modified logger.
"""
ray_logger = logging.getLogger("ray")
ray_logger.propagate = True
ray_logger.handlers.clear()
ray._private.log.clear_logger("ray")

logger = logging.getLogger(logger_name)
if type(logging_level) is str:
Expand Down
9 changes: 9 additions & 0 deletions python/ray/tests/test_logging.py
Original file line number Diff line number Diff line change
Expand Up @@ -942,6 +942,15 @@ def test_log_level_settings(
assert len(caplog.records) == 0, "Log message found where none are expected."


def test_log_with_import():

logger = logging.getLogger(__name__)
assert not logger.disabled
ray.log.logger_initialized = False
ray.log.generate_logging_config()
assert not logger.disabled


if __name__ == "__main__":
import sys

Expand Down
Loading