fix(log): show warnings when logging is not configured by hippi345 · Pull Request #791 · redis/redis-vl-python

hippi345 · 2026-10-27T20:52:23Z

Motivation

Fixes #786.

RedisVL warnings never reach an application that has not configured logging, including warnings that signal lost data.

get_logger attached a logging.NullHandler whenever a logger had no handlers. On Python 3.2+, a WARNING record with no handler in the hierarchy normally goes to logging.lastResort (stderr), but a NullHandler counts as a handler, so the record was dropped. Pytest’s caplog installs its own handler, which is why tests on main did not catch this.

Changes

Default stderr logging in `get_logger`

get_logger now attaches a _DefaultStderrHandler instead of a NullHandler. When the host application has not configured logging, WARNING and above are forwarded to stderr through logging.lastResort; INFO and DEBUG stay quiet. The handler strips exc_info, exc_text, and stack_info so default stderr output does not dump tracebacks on top of CLI error lines.

_has_application_handlers and _default_stderr_owner decide when the default handler emits and prevent duplicate lines for parent/child RedisVL loggers. Loggers keep default propagation (propagate=True) so once the app adds real handlers (root basicConfig, a parent logger handler, and so on), the default handler stops emitting and records propagate normally.

CLI failure logging

log_cli_exception() in redisvl/cli/utils.py calls logger.error(..., exc_info=True) only when _has_application_handlers is true. rvl index, rvl stats, and rvl migrate use it so unconfigured runs still print a single error line on stderr without a duplicate traceback.

Tests

tests/unit/test_log.py exercises logging in fresh interpreter subprocesses (pytest caplog would mask the bug).
tests/unit/test_cli_index.py asserts Traceback is absent from stderr on runtime failures.

Notes

Loggers that already had a NullHandler attached earlier in the same process keep it; this change does not remove third-party handlers.

Release Notes

RedisVL warnings and errors now show on stderr when the application has not configured logging.

Reviewer Brief

Start here: redisvl/utils/log.py, _DefaultStderrHandler.emit. It gates unconfigured-app output: when to forward WARNING+ to lastResort, when to strip tracebacks, and when to defer to the host’s logging config.

Where it could fail

redisvl/utils/log.py:49: _has_application_handlers must match real host setups (root basicConfig, parent handlers, and so on); a misclassification leaves WARNING+ on stderr when the app also logs, or skips stderr when the app expected lastResort.
redisvl/utils/log.py:37: _default_stderr_owner must pick one handler in a get_logger hierarchy; the wrong owner could suppress warnings or print them twice.
redisvl/cli/utils.py:15: log_cli_exception uses _has_application_handlers on [RedisVL]; misclassification duplicates tracebacks on stderr or skips structured logging.

Not verified: Full make test with the default tests/conftest.py session fixture (needs Docker). Python versions other than 3.12.3 in this review environment.

Beyond CI: On ba9f127, make lint passed (black/isort unchanged, mypy clean on 122 files). With --confcutdir=tests/unit (no Docker on the review host), tests/unit/test_log.py — 8 passed; tests/unit/test_cli_index.py test_index_runtime_errors — 7 passed (15 total). On origin/main (31d2d0a), the same subprocess reproduction prints only from plain logger on stderr (redisvl warning missing). With the PR’s test_log.py on main (import adjusted to omit _DefaultStderrHandler), the three unconfigured-stderr subprocess tests fail; four configured-logging cases pass.

_{Reviewed ba9f127 · Cursor agent (composer-2.5) · lenses: correctness, security, design, tests, docs, Redis behaviour}

get_logger attached a NullHandler, which counts as a handler and stops Python from sending WARNING and above to logging.lastResort. Drop it so an application that has not configured logging still sees RedisVL warnings on stderr. Fixes redis#786

cursor

Stale Bugbot comment from a previous run.

vishal-bala

Hi, please follow the current contribution guidelines and include a "Reviewer Brief" in your description (based on a pre-PR review using the skill included in the repository).

Co-authored-by: Joel Shearon <[email protected]>

Attach a default stderr handler on get_logger loggers so unconfigured apps still see WARNING+ on stderr (redis#786) while exc_info does not dump tracebacks there. CLI exception handlers only log when the host app configured logging.

cursor

Stale Bugbot comment from a previous run.

Stop forcing propagate=False on RedisVL loggers. The default stderr handler now no-ops when the application attached handlers on the root or a parent logger, so INFO/DEBUG and ancestor handlers work again while unconfigured WARNING+ output still reaches stderr without exc tracebacks.

hippi345 · 2026-10-31T00:35:28Z

good catch, dropped the propagate=False and the default handler now only kicks in when the app hasn't set up its own logging. added tests for basicConfig and a parent logger handler. fixed in da3b4c1.

cursor

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

^{Reviewed by Cursor Bugbot for commit da3b4c1. Configure here.}

Only the innermost _DefaultStderrHandler for a record emits to lastResort so parent/child get_logger calls do not duplicate warnings. Skip emit when logging.lastResort is None per stdlib semantics.

vishal-bala · 2026-11-02T13:29:05Z

Hi, your reviewer brief doesn't meet the expectations that the repository and the skill set out. If you're using an agent, it should be clear enough for them to run the subagent review panel with the lenses expected.

When your PR has gone through the proper preparation, we can have a human reviewer take a look at it 👍

hippi345 · 2026-11-12T06:54:07Z

Hi @vishal-bala — thanks for calling out the earlier brief. I re-ran preparation against head ba9f127: full pre-PR review, make lint, and the touched unit tests. Lint and mypy are clean; all 8 tests in tests/unit/test_log.py and all 7 test_index_runtime_errors cases pass locally (--confcutdir=tests/unit here because this environment has no Docker; CI still uses the normal conftest path). I also checked origin/main: the subprocess reproduction omits the RedisVL warning, and the new subprocess tests fail there on the three unconfigured-stderr cases. The PR description now follows the template with an updated Reviewer Brief. Ready for a human pass when you are.

vishal-bala · 2026-11-12T08:55:44Z

 import logging


+class _DefaultStderrHandler(logging.Handler):


Could this go back to the fix proposed in #786, which drops the NullHandler and attaches nothing? With no handler on a RedisVL logger, the stdlib's lastResort already does everything this handler re-creates. I measured both on 3.13:

INFO stays hidden.

A child warning and a parent warning each print once.

A root or parent handler takes over once the app configures logging.

An existing NullHandler still silences the logger.

The plain fix leaves one problem, in the CLI (see the comment on cli/utils.py). Its cause is that each call site both logs and prints the same error, so it's better fixed there. A handler like this one is something every later logging change has to reason about, and it adds costs of its own:

It can deadlock. emit() runs while the handler's lock is held, and it calls logging.getLogger(), which takes logging._lock. dictConfig takes the two locks in the opposite order: it takes logging._lock first, then acquires every handler's lock in _clearExistingHandlers(). So a RedisVL warning on one thread and a dictConfig call on another can block each other for good. I reproduced it on 3.13.9 by adding a sleep in emit() to widen the window.

Two different unconfigured behaviours. Because the handler strips exc_info, get_logger loggers behave differently from the plain logging.getLogger loggers in redisvl/mcp/, which keep their tracebacks.

Against stdlib guidance. The Python logging HOWTO tells libraries to add no handler other than NullHandler, and to leave output to the application.

get_logger would then be:

def get_logger(name, log_level="info", fmt=None): """Return a logger instance.""" name = "RedisVL" if log_level == "debug" else name return logging.getLogger(name)

One docs line also needs updating. docs/concepts/queries.md:357 says RedisVL "silently skips" a document whose fields came back empty, while it logs a WARNING. That warning now reaches stderr.

vishal-bala · 2026-11-12T08:55:44Z

 DEFAULT_REDIS_PORT = 6379


+def log_cli_exception(exc: BaseException) -> None:


log_cli_exception never logs in a real rvl process. Nothing under redisvl/ configures logging, so _has_application_handlers("[RedisVL]") is always False there (measured). It only fires when the CLI classes run inside a process that already has handlers, such as pytest.

Could the three call sites just drop their log call instead?

In cli/index.py:86 and cli/stats.py:116, delete logger.error(e, exc_info=True) and keep the print.

In cli/migrate.py:77, delete logger.error(e).

Once the NullHandler is gone, that gives the same single error line this PR prints, without importing a private helper from log.py.

vishal-bala · 2026-11-12T08:55:44Z

    assert exc_info.value.code == 1
    assert captured.out == ""
    assert captured.err != ""
+    assert "Traceback" not in captured.err


This assertion passes on main as well. Under pytest the root logger holds caplog's handlers, so the record never reaches stderr, whatever log_cli_exception does. Could the CLI case be a subprocess test instead? It would run a failing command with logging unconfigured and assert that stderr has one error line and no Traceback.

vishal-bala · 2026-11-12T08:55:44Z

+    )
+
+
+def test_get_logger_attaches_default_stderr_handler():


Once the handler is gone, most of this file can go too. Two subprocess tests would pin #786:

With logging unconfigured, a WARNING reaches stderr and an INFO doesn't.

After basicConfig(), the warning appears exactly once.

The tests that import _DefaultStderrHandler or count handlers check implementation details rather than behaviour.

cursor Bot reviewed Oct 27, 2026

View reviewed changes

Comment thread redisvl/utils/log.py Resolved

vishal-bala requested changes Oct 30, 2026

View reviewed changes

vishal-bala self-assigned this Oct 30, 2026

hippi345 added 2 commits October 30, 2026 14:50

Merge branch 'main' of github.com:redis/redis-vl-python into fix-786

7c73fae

Co-authored-by: Joel Shearon <[email protected]>

hippi345 force-pushed the fix-786 branch from 149a8e6 to 6a55dc7 Compare October 30, 2026 18:50

cursor Bot reviewed Oct 30, 2026

View reviewed changes

Comment thread redisvl/utils/log.py Resolved

cursor Bot reviewed Oct 31, 2026

View reviewed changes

Comment thread redisvl/utils/log.py Resolved

Comment thread redisvl/utils/log.py Resolved

fix(log): guard lastResort and dedupe hierarchical stderr handlers

ba9f127

Only the innermost _DefaultStderrHandler for a record emits to lastResort so parent/child get_logger calls do not duplicate warnings. Skip emit when logging.lastResort is None per stdlib semantics.

vishal-bala requested changes Nov 12, 2026

View reviewed changes

		DEFAULT_REDIS_PORT = 6379


		def log_cli_exception(exc: BaseException) -> None:

Conversation

hippi345 commented Oct 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Default stderr logging in get_logger

CLI failure logging

Tests

Notes

Release Notes

Reviewer Brief

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vishal-bala left a comment

Choose a reason for hiding this comment

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hippi345 commented Oct 31, 2026

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vishal-bala commented Nov 2, 2026

Uh oh!

hippi345 commented Nov 12, 2026

Uh oh!

vishal-bala Nov 12, 2026

Choose a reason for hiding this comment

Uh oh!

vishal-bala Nov 12, 2026

Choose a reason for hiding this comment

Uh oh!

vishal-bala Nov 12, 2026

Choose a reason for hiding this comment

Uh oh!

vishal-bala Nov 12, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hippi345 commented Oct 27, 2026 •

edited

Loading

Default stderr logging in `get_logger`

cursor Bot left a comment •

edited

Loading

cursor Bot left a comment •

edited

Loading