feat: add logging reset support#808
Conversation
Derive logging state from managed handlers and remove the interface runtime latch so automatic configuration is evaluated per instance. Add reset_logging(), regression coverage, and user documentation. Closes #805 Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Close Data Designer-managed handlers during reset so file resources are released. Inline idempotent runtime setup in DataDesigner and verify behavior through the public constructor. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
|
Fern preview: https://nvidia-preview-pr-808.docs.buildwithfern.com/nemo/datadesigner
|
Code Review: PR #808 — feat: add logging reset supportAuthor: nabinchha (Nabin Mulepati) · Base: SummaryThis PR replaces process-global boolean latches (
This is a clean, well-motivated design. Deriving state from observable reality FindingsCorrectness
Conventions / Style
Test Coverage
Documentation
Minor / Optional
Structural Impact(graphify, 2.4s) Risk: MEDIUM (high-connectivity entity (DataDesigner, 86 deps))
High-Connectivity Changes
Cross-Package Dependencies
Reviewer note on blast radius: The MEDIUM risk stems from touching VerdictApprove with minor, non-blocking suggestions. This is a solid refactor that Note: |
Greptile SummaryThis PR replaces process-global boolean latches (
|
| Filename | Overview |
|---|---|
| packages/data-designer-config/src/data_designer/logging.py | Introduces DataDesignerManagedHandler marker mixin and two concrete handler subclasses; replaces process-global bool with handler-presence detection; adds reset_logging(). configure_logging() now closes all root handlers (including foreign ones) rather than merely detaching them — a behaviour change that was flagged in a prior review thread. |
| packages/data-designer/src/data_designer/interface/data_designer.py | Removes _initialize_interface_runtime one-shot function; inlines per-construction configure_logging guard and resolve_seed_default_model_settings call into init. Logic is correct and matches documented per-instance semantics. |
| packages/data-designer-config/tests/test_logging.py | Replaces monkeypatch-of-private-global tests with real-state tests using reset_logging(); adds comprehensive coverage of reset semantics, foreign-handler preservation, idempotency, and file-handler close behaviour. |
| packages/data-designer/tests/interface/test_data_designer.py | Old monkeypatched _initialize_interface_runtime tests removed; new tests verify per-instance logging, handler stability, and per-construction resolve_seed_default_model_settings. Adds auto_configure_logging=False to the caplog-dependent test to preserve pytest's capture handler. |
| fern/versions/latest/pages/concepts/logging.mdx | New Fern concept page documenting configure_logging, auto_configure_logging opt-out, and reset_logging. Content accurately reflects implementation semantics. |
| fern/versions/latest.yml | Adds Logging navigation entry pointing to the new logging.mdx concept page. Trivial config change. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant App
participant DD as DataDesigner.__init__
participant IL as is_logging_configured
participant CL as configure_logging
participant RL as reset_logging
participant Root as Root Logger
App->>DD: DataDesigner()
DD->>IL: check handlers
IL->>Root: any DataDesignerManagedHandler?
Root-->>IL: False
IL-->>DD: False
DD->>CL: configure_logging()
CL->>Root: remove+close all existing handlers
CL->>Root: addHandler DataDesignerStreamHandler
CL->>Root: setLevel INFO
App->>DD: DataDesigner() second construction
DD->>IL: check handlers
Root-->>IL: True managed handler present
IL-->>DD: True
DD-->>App: skip configure_logging idempotent
App->>RL: reset_logging()
RL->>Root: find managed handlers
Root-->>RL: DataDesignerStreamHandler list
RL->>Root: removeHandler+close each managed handler
RL->>Root: setLevel WARNING
RL->>Root: data_designer setLevel NOTSET
App->>DD: DataDesigner() after reset
DD->>IL: check handlers
Root-->>IL: False no managed handlers
IL-->>DD: False
DD->>CL: configure_logging() again
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant App
participant DD as DataDesigner.__init__
participant IL as is_logging_configured
participant CL as configure_logging
participant RL as reset_logging
participant Root as Root Logger
App->>DD: DataDesigner()
DD->>IL: check handlers
IL->>Root: any DataDesignerManagedHandler?
Root-->>IL: False
IL-->>DD: False
DD->>CL: configure_logging()
CL->>Root: remove+close all existing handlers
CL->>Root: addHandler DataDesignerStreamHandler
CL->>Root: setLevel INFO
App->>DD: DataDesigner() second construction
DD->>IL: check handlers
Root-->>IL: True managed handler present
IL-->>DD: True
DD-->>App: skip configure_logging idempotent
App->>RL: reset_logging()
RL->>Root: find managed handlers
Root-->>RL: DataDesignerStreamHandler list
RL->>Root: removeHandler+close each managed handler
RL->>Root: setLevel WARNING
RL->>Root: data_designer setLevel NOTSET
App->>DD: DataDesigner() after reset
DD->>IL: check handlers
Root-->>IL: False no managed handlers
IL-->>DD: False
DD->>CL: configure_logging() again
Reviews (8): Last reviewed commit: "Merge branch 'main' into codex/805-reset..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
now that |
tiny doc nit: "skipped when |
Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
|
Addressed both new comments in e5a8d09:
Verification: 106 targeted tests passed; Ruff lint and format checks passed. |
|
Replying to the handler lifecycle comment: addressed in |
|
Replying to the constructor docstring comment: addressed in |
Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Resolve conflict in data_designer.py by keeping PR's inlined logging configuration (for reset support) alongside main's OpenTelemetry runtime initialization and _observe_create decorator.
Address review feedback by including is_logging_configured in the documented import from data_designer.logging.
| # Remove and close all handlers replaced by this configuration. | ||
| for handler in root_logger.handlers[:]: | ||
| root_logger.removeHandler(handler) | ||
| handler.close() |
There was a problem hiding this comment.
configure_logging() closes foreign handlers it doesn't own
configure_logging() iterates over every root-logger handler and calls .close(), including handlers installed by embedding applications. The old implementation used handlers.clear(), which merely detached them from the root logger without closing the underlying resources. The new code closes any FileHandler — or StreamHandler whose stream is e.g. sys.stderr — that a third-party added. An embedding app that holds a direct reference to such a handler and continues emitting records will get a ValueError: I/O operation on closed file after configure_logging() runs. Only DataDesignerManagedHandler instances need to be closed; foreign handlers should be removed from the root logger but left open, matching the behavior reset_logging() already implements correctly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/data-designer-config/src/data_designer/logging.py
Line: 134-137
Comment:
**`configure_logging()` closes foreign handlers it doesn't own**
`configure_logging()` iterates over every root-logger handler and calls `.close()`, including handlers installed by embedding applications. The old implementation used `handlers.clear()`, which merely detached them from the root logger without closing the underlying resources. The new code closes any `FileHandler` — or `StreamHandler` whose stream is e.g. `sys.stderr` — that a third-party added. An embedding app that holds a direct reference to such a handler and continues emitting records will get a `ValueError: I/O operation on closed file` after `configure_logging()` runs. Only `DataDesignerManagedHandler` instances need to be closed; foreign handlers should be removed from the root logger but left open, matching the behavior `reset_logging()` already implements correctly.
How can I resolve this? If you propose a fix, please make it concise.
|
CI was failing in multiple PRs due to some weird issue, I just merged #814 which is supposed to fix it. Rebased to see if it helps |
3 participants
📋 Summary
Replace process-global runtime initialization latches with observable logging state and idempotent per-construction setup. Add a public
reset_logging()API so embedding applications can surgically undo Data Designer logging without disturbing foreign handlers; missing default model settings are re-resolved safely on each construction.🔗 Related Issue
Closes #805
🔄 Changes
is_logging_configured()from the handlers currently attached to the root logger.reset_logging()to remove and close managed handlers, preserve foreign handlers, and restore default root anddata_designerlevels.auto_configure_loggingis evaluated for eachDataDesignerconstruction. An explicitFalseapplies only to that instance; a later default construction intentionally opts back into Data Designer logging.resolve_seed_default_model_settings()on each construction; the operation remains idempotent and writes only missing defaults.🔍 Attention Areas
data_designer.logging— public managed-handler and reset semantics.DataDesigner.__init__— per-construction logging and default-settings initialization.🧪 Testing
make testequivalent passes: config (627), engine (2,172), interface (1,037 passed, 1 skipped)✅ Checklist
Description updated with AI