Skip to content

fix(interface): prevent configure_logging from being silently overwritten #408

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.

Sign up for GitHub

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

Open
anzzyspeaksgit wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(interface): prevent configure_logging from being silently overwritten #408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab533d1
fix(interface): prevent configure_logging from being silently overwri…
anzzyspeaksgit Mar 13, 2026
8c352db
fix(interface): address greptile code review comments on logging over…
anzzyspeaksgit Mar 14, 2026
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
6 changes: 6 additions & 0 deletions packages/data-designer-config/src/data_designer/logging.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,11 @@ def working() -> str:
return random.choice(["⚙️", "🔧", "🔨", "⚒️", "🛠️", "💼", "👷", "🏗️", "🪛", "👨‍💻"])


_configured = False


def configure_logging(config: LoggingConfig | None = None) -> None:
Comment on lines +117 to 120
Copy link
Contributor

@greptile-apps greptile-apps bot Mar 13, 2026

Choose a reason for hiding this comment

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

Missing blank line before top-level function (PEP 8)

PEP 8 requires two blank lines between top-level definitions. Currently only one blank line separates _configured = False from def configure_logging(...).

Suggested change
_configured = False
def configure_logging(config: LoggingConfig | None = None) -> None:
_configured = False
def configure_logging(config: LoggingConfig | None = None) -> None:
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: 117-119

Comment:
**Missing blank line before top-level function (PEP 8)**

PEP 8 requires two blank lines between top-level definitions. Currently only one blank line separates `_configured = False` from `def configure_logging(...)`.

```suggestion
_configured = False

def configure_logging(config: LoggingConfig | None = None) -> None:
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

global _configured
config = config or LoggingConfig.default()

root_logger = logging.getLogger()
Expand All @@ -137,6 +141,8 @@ def configure_logging(config: LoggingConfig | None = None) -> None:
for name in config.to_silence:
quiet_noisy_logger(name)

_configured = True
Copy link
Contributor

@greptile-apps greptile-apps bot Mar 14, 2026

Choose a reason for hiding this comment

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

_configured flag is set for runtime-initiated calls too

After _initialize_interface_runtime() calls configure_logging() (the default setup path), _configured becomes True. This means the flag no longer exclusively tracks "the user explicitly called configure_logging()" — it conflates user-initiated configuration with runtime-initiated configuration.

In practice this doesn't cause a functional bug because the _interface_runtime_initialized guard prevents _initialize_interface_runtime() from re-evaluating the _configured check after the first run. However, if the flag is ever read in a future context expecting to know whether the user configured logging (rather than just whether any call to configure_logging() occurred), the semantics will be misleading.

Consider documenting this clearly, or renaming the flag to _logging_setup_complete to reflect its actual meaning: logging was configured (by anyone), so skip default setup.

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: 144

Comment:
**`_configured` flag is set for runtime-initiated calls too**

After `_initialize_interface_runtime()` calls `configure_logging()` (the default setup path), `_configured` becomes `True`. This means the flag no longer exclusively tracks "the user explicitly called `configure_logging()`" — it conflates user-initiated configuration with runtime-initiated configuration.

In practice this doesn't cause a functional bug because the `_interface_runtime_initialized` guard prevents `_initialize_interface_runtime()` from re-evaluating the `_configured` check after the first run. However, if the flag is ever read in a future context expecting to know whether the *user* configured logging (rather than just whether *any* call to `configure_logging()` occurred), the semantics will be misleading.

Consider documenting this clearly, or renaming the flag to `_logging_setup_complete` to reflect its actual meaning: logging was configured (by anyone), so skip default setup.

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!



def quiet_noisy_logger(name: str) -> None:
logger = logging.getLogger(name)
Expand Down
8 changes: 8 additions & 0 deletions packages/data-designer-config/tests/test_logging.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@
quiet_noisy_logger,
)

import data_designer.logging as _logging_mod

@pytest.fixture(autouse=True)
def reset_configured_flag():
original = _logging_mod._configured
yield
_logging_mod._configured = original
Comment on lines +22 to +26
Copy link
Contributor

@greptile-apps greptile-apps bot Mar 14, 2026

Choose a reason for hiding this comment

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

reset_configured_flag fixture restores but does not reset to a known baseline

The fixture captures the current value of _configured at fixture setup and restores it on teardown. This is a clean teardown pattern, but it does not guarantee _configured = False at the start of each test.

If the module is imported in a process where configure_logging() was already called before the test session starts (e.g., by a conftest or another test module), original will be True and any test that implicitly relies on _configured starting as False will see True throughout.

Adding an explicit reset to False at the start of the fixture (before yield) would make the test isolation unconditional:

@pytest.fixture(autouse=True)
def reset_configured_flag():
    original = _logging_mod._configured
    _logging_mod._configured = False  # ensure clean slate for each test
    yield
    _logging_mod._configured = original
Prompt To Fix With AI 
This is a comment left during a code review.
Path: packages/data-designer-config/tests/test_logging.py
Line: 22-26

Comment:
**`reset_configured_flag` fixture restores but does not reset to a known baseline**

The fixture captures the *current* value of `_configured` at fixture setup and restores it on teardown. This is a clean teardown pattern, but it does not guarantee `_configured = False` at the *start* of each test.

If the module is imported in a process where `configure_logging()` was already called before the test session starts (e.g., by a conftest or another test module), `original` will be `True` and any test that implicitly relies on `_configured` starting as `False` will see `True` throughout.

Adding an explicit reset to `False` at the start of the fixture (before `yield`) would make the test isolation unconditional:

```python
@pytest.fixture(autouse=True)
def reset_configured_flag():
    original = _logging_mod._configured
    _logging_mod._configured = False  # ensure clean slate for each test
    yield
    _logging_mod._configured = original
```

How can I resolve this? If you propose a fix, please make it concise.



@pytest.fixture
def stub_default_logging_config():
Expand Down
4 changes: 3 additions & 1 deletion packages/data-designer/src/data_designer/interface/data_designer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,9 @@ def _initialize_interface_runtime() -> None:
global _interface_runtime_initialized
if _interface_runtime_initialized:
return
configure_logging()
from data_designer import logging as _logging_mod
if not _logging_mod._configured:
configure_logging()
resolve_seed_default_model_settings()
_interface_runtime_initialized = True

Expand Down
19 changes: 19 additions & 0 deletions packages/data-designer/tests/interface/test_data_designer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,9 @@ def test_validate_raises_error_when_seed_collides(
def test_initialize_interface_runtime_runs_once(monkeypatch: pytest.MonkeyPatch) -> None:
"""_initialize_interface_runtime only runs initialization once."""
monkeypatch.setattr(dd_mod, "_interface_runtime_initialized", False)

from data_designer import logging as _logging_mod
monkeypatch.setattr(_logging_mod, "_configured", False)

with (
patch("data_designer.interface.data_designer.configure_logging") as mock_logging,
Expand All @@ -464,3 +467,19 @@ def test_initialize_interface_runtime_runs_once(monkeypatch: pytest.MonkeyPatch)
dd_mod._initialize_interface_runtime()
mock_logging.assert_called_once()
mock_resolve.assert_called_once()


def test_initialize_interface_runtime_skips_logging_if_already_configured(monkeypatch: pytest.MonkeyPatch) -> None:
"""_initialize_interface_runtime skips logging config if user already configured it."""
monkeypatch.setattr(dd_mod, "_interface_runtime_initialized", False)

from data_designer import logging as _logging_mod
monkeypatch.setattr(_logging_mod, "_configured", True)

with (
patch("data_designer.interface.data_designer.configure_logging") as mock_logging,
patch("data_designer.interface.data_designer.resolve_seed_default_model_settings") as mock_resolve,
):
dd_mod._initialize_interface_runtime()
mock_logging.assert_not_called()
mock_resolve.assert_called_once()
Loading