fix: preserve preconfigured logging#755
Conversation
Signed-off-by: schultzjack <schultzjack@users.noreply.github.com>
|
All contributors have signed the DCO ✍️ ✅ |
Greptile SummaryThis PR fixes a bug where
|
| Filename | Overview |
|---|---|
| packages/data-designer-config/src/data_designer/logging.py | Adds module-level _logging_configured flag, sets it at the end of configure_logging(), and exposes is_logging_configured() predicate — straightforward and correct. |
| packages/data-designer/src/data_designer/interface/data_designer.py | Guards the default configure_logging() call in _initialize_interface_runtime() behind is_logging_configured() — clean one-line change with no unintended side effects. |
| packages/data-designer-config/tests/test_logging.py | Adds a focused test for the new _logging_configured flag using monkeypatch for proper isolation. |
| packages/data-designer/tests/interface/test_data_designer.py | Adds regression test for preconfigured-logging preservation and fixes the existing test_initialize_interface_runtime_runs_once by resetting _logging_configured — necessary to keep the mock assertion reliable. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant logging as data_designer.logging
participant iface as _initialize_interface_runtime
User->>logging: configure_logging(LoggingConfig.debug())
logging->>logging: set handlers, levels
logging->>logging: "_logging_configured = True"
User->>iface: DataDesigner(...)
iface->>iface: _interface_runtime_initialized? → False
iface->>logging: is_logging_configured()
logging-->>iface: True
iface->>iface: skip configure_logging() ✓
iface->>iface: resolve_seed_default_model_settings()
iface->>iface: "_interface_runtime_initialized = True"
iface-->>User: DataDesigner ready (DEBUG logging preserved)
%%{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 User
participant logging as data_designer.logging
participant iface as _initialize_interface_runtime
User->>logging: configure_logging(LoggingConfig.debug())
logging->>logging: set handlers, levels
logging->>logging: "_logging_configured = True"
User->>iface: DataDesigner(...)
iface->>iface: _interface_runtime_initialized? → False
iface->>logging: is_logging_configured()
logging-->>iface: True
iface->>iface: skip configure_logging() ✓
iface->>iface: resolve_seed_default_model_settings()
iface->>iface: "_interface_runtime_initialized = True"
iface-->>User: DataDesigner ready (DEBUG logging preserved)
Reviews (1): Last reviewed commit: "fix: preserve preconfigured logging" | Re-trigger Greptile
|
recheck |
Stale PR reminderThis PR has had failing checks for 7 days without activity. Failing checks: check Please push an update or leave a comment if you're still working on this. To prevent auto-close, add the |
|
Issue #388 has been triaged. The linked issue check is being re-evaluated. |
|
Thanks for jumping on this, this fixes a pretty easy-to-miss logging footgun. I reviewed the runtime init path and the new logging state tracking. The core behavior looks good, but I'd like one small clarification before merge.
Please add a short docstring/comment making that scope explicit, or broaden the check if you want this to cover general Python logging setup too. The current fix is fine for the linked issue, but the helper name reads a bit broader than what it actually guarantees. Focused tests and a smoke check passed locally. Once that expectation is tightened up, this looks good to merge from my side. |
|
Following up on my comment above: the current fix handles the narrow repro from #388 where the caller uses There is a related case discussed later in #388 that this does not cover yet: applications that configure logging through stdlib APIs before using Data Designer. For example, if a caller uses I think the lowest-risk incremental direction is to make DD's automatic runtime logging setup opt-out explicitly, which also matches the direction Mike suggested on #388. Something like:
That should keep standalone behavior unchanged while giving embedded/library users a reliable way to prevent DD from taking over process logging. |
Summary
Testing
Closes #388