NVIDIA-NeMo / NVIDIA-NeMo/Guardrails

refactor: stop configuring logging from library code

Open
#2,314 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
7.2k
Forks
842
Avg merge
3d 1h
Merged PRs (30d)
25

Description

Required checks

  • I searched existing issues and pull requests for related refactor proposals.
  • I understand that opening this issue does not mean a refactor PR will be accepted.
  • I understand that a refactor PR should not be opened unless a maintainer approves the plan and assigns the work.

Problem

The package configures logging on behalf of the application that imports it, in several places. The Python logging HOWTO's Configuring Logging for a Library is explicit that libraries should emit records and leave handlers, levels, and formatting to the application, adding only a NullHandler to their top-level logger. We do the opposite in three distinct ways.

Root logger mutation. nemoguardrails/server/api.py:87 calls logging.basicConfig(level=logging.INFO) at module import time, so merely importing the server module reconfigures logging for the whole process. nemoguardrails/logging/verbose.py:194-205 raises the root logger's level, adds a handler to root, and walks the application's existing root StreamHandlers demoting them to WARNING. The CLI and eval entry points (cli/__init__.py:46,195,242, evaluate/cli/evaluate.py:29, eval/cli.py:32) also set the root level; those are defensible, since a CLI is an application, but the first two are library code.

Detaching a subtree from the application. configure_logging() (guardrails/__init__.py) attaches a handler and sets propagate = False on the nemoguardrails.guardrails logger. Any handler the application installed on root then stops receiving that subtree entirely, silently. On the repeat-call path it also calls setLevel/setFormatter on handlers it does not own, reformatting the application's handlers. PR #2310 fixed one instance of this (it was being called on every Guardrails construction, including verbose=False), but the pattern remains.

verbose: bool as the public knob. A boolean has to be mapped to a level somewhere, and that mapping is a private decision that users cannot see or predict. The two entry points chose differently and drifted apart without anyone noticing: LLMRails(verbose=True) calls set_verbose(True, llm_calls=True)VerboseHandler at INFO on root, while Guardrails(verbose=True) calls configure_logging(logging.DEBUG) → stderr handler on nemoguardrails.guardrails at DEBUG. IORails takes no verbose parameter at all, so on the fallback path to LLMRails both apply, and on the IORails path only the first does. This was raised in review on PR #2310 (thanks @Pouyanpi) and is documented as of that PR, but documenting a divergence is not the same as removing it.

The flag also bundles decisions that are not the same question: how much detail (level), where it goes (handler), how it looks (formatter), who else sees it (propagation), and whether LLM prompts and completions are logged. That last one is a content decision with PII implications, not a verbosity level, and deserves to be switchable on its own.

Practical consequences: applications lose log records they configured for and get no error saying so; a library-owned propagate = False is invisible to caplog, which is how this surfaced as an order-dependent test failure in PR #2310; and no combination of these concerns can be requested independently.

Proposed direction

Move logging configuration out of the library and let the logger hierarchy do the filtering.

  • Add a NullHandler to the top-level nemoguardrails logger so the package is silent by default without suppressing application configuration.
  • Remove import-time and constructor-time configuration from library modules. Keep configure_logging() as an explicit, documented, opt-in helper — the helper is fine, calling it for the user is not.
  • Confine root-logger configuration to genuine application entry points (CLI, eval CLI, and the server only when it owns the process, not on import).
  • Replace verbose: bool on library constructors with either a level-shaped argument (log_level: int | str | None = None, default None meaning "leave logging alone") or nothing at all, relying on the hierarchy: logging.getLogger("nemoguardrails.<area>").setLevel(...). Keep any -v/-vv → level mapping at the CLI boundary.
  • Separate console UX from diagnostics. set_verbose() printing "Entered verbose mode", installing a RichHandler, and demoting the application's handlers is terminal presentation, not logging; it is what pushes the library into mutating root.
  • Review logger naming so the areas users actually want to filter on (rail decisions, LLM calls, HTTP) are addressable by name.

Deprecate verbose.

Migration and compatibility notes

verbose is public on both LLMRails and Guardrails and appears in docs and examples, so it needs a deprecation path rather than removal: keep accepting it, map it to the new argument, and warn via warnings.warn(..., DeprecationWarning, stacklevel=...) per nemoguardrails/AGENTS.md.

Users who today rely on console output appearing by default would need to configure logging themselves. PR #2310 already made this change for the default Guardrails(...) path; extending it to the verbose paths and to set_verbose widens the user-visible surface, so it likely wants a release note and a migration snippet (logging.basicConfig(level=logging.INFO)).

Docs affected: docs/observability/logging/index.mdx (the verbose-mode section, including the comparison table added in #2310) and docs/observability/tracing/opentelemetry-logs.mdx (the propagate=False note, which could be deleted outright if the library stops setting it).

Server behavior needs care: removing the import-time basicConfig means the server must configure logging when it starts rather than when it is imported, so deployments do not silently lose server logs.

Validation plan

  • Tests asserting the library adds no handlers, changes no levels, and leaves propagate untouched on import and on construction of each public entry point.
  • A test that importing nemoguardrails.server.api does not mutate root logger level or handlers.
  • Tests that records from each area reach a handler the application attached to root, which is the property that actually broke.
  • Deprecation tests for verbose on both entry points.
  • Existing suites for the CLI, server, and eval paths to confirm operator-visible output is unchanged where intended.
  • make docs-fern, plus updates to the two docs pages above.
  • Worth running under make test WORKERS=1 as well as the parallel default: the failure mode here is order-dependent global state, which the parallel runner can mask.

Risks

The main risk is user-visible quiet: anyone depending on default console output loses it, and unlike an API break it fails silently. Mitigation is the deprecation window plus an explicit release note and migration snippet.

set_verbose() mutates module-level globals (verbose_mode_enabled, debug_mode_enabled, verbose_llm_calls) and is not idempotent across instances, so untangling it may reach further into the Colang runtime than the logging change alone suggests. That argues for splitting the work: the NullHandler and import-time basicConfig fixes are small and low-risk, while the verbose redefinition is a larger, separately-scoped change.

Alternatives considered: leave it as-is and document the differences, which is what PR #2310 does — cheap, but the silent-loss behavior for embedding applications remains; or keep verbose and only align the two entry points on one level, which fixes the reported inconsistency without addressing why a library is configuring logging at all.

Open question: whether the server should configure logging at all, or whether that belongs to the deployment (uvicorn/gunicorn config).

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reviewing the logging sites named in nemoguardrails/server/api.py, nemoguardrails/logging/verbose.py, guardrails/init.py, and the CLI and eval entry points, along with nemoguardrails/AGENTS.md. Use the proposed validation tests as the acceptance map: no unexpected handler, level, or propagation changes, root records remain reachable, and verbose deprecation behavior is covered. Confirm the scope with a maintainer before splitting or implementing the work.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
observability
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.