Skip to content
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

Jump to bottom

Only convert omegaconf DictConfig to primitive type for logging #2176

Merged
merged 7 commits into from Jan 10, 2023
Merged

Only convert omegaconf DictConfig to primitive type for logging #2176

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e2a00f5
Only convert omegaconf DictConfig to primitive type for logging
merelcht Jan 5, 2023
ab003c1
Check if dealing with omegaconf dict before converting
merelcht Jan 5, 2023
67ff24d
Add test to check logging file is parsed correctly with omegaconfloader
merelcht Jan 5, 2023
e0678e1
Merge branch 'main' into fix/omegaconfloader-logging
jmholzer Jan 9, 2023
f8172a2
Address review suggestion
merelcht Jan 10, 2023
8828bdd
Merge branch 'main' into fix/omegaconfloader-logging
merelcht Jan 10, 2023
e87c320
Fix lint
merelcht Jan 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 2 additions & 3 deletions kedro/config/omegaconf_config.py
View file
Open in desktop
Expand Up @@ -233,9 +233,8 @@ def load_and_merge_dir_config(self, conf_path: str, patterns: Iterable[str]):

if aggregate_config:
if len(aggregate_config) > 1:
merged_config = OmegaConf.merge(*aggregate_config)
return OmegaConf.to_container(merged_config)
return OmegaConf.to_container(list(aggregate_config)[0])
return dict(OmegaConf.merge(*aggregate_config))
return list(aggregate_config)[0]
return {}
merelcht marked this conversation as resolved.
Show resolved Hide resolved

@staticmethod
Expand Down
3 changes: 3 additions & 0 deletions kedro/framework/session/session.py
View file
Open in desktop
Expand Up @@ -10,6 +10,7 @@
from typing import Any, Dict, Iterable, Optional, Union

import click
from omegaconf import OmegaConf, omegaconf

from kedro import __version__ as kedro_version
from kedro.config import ConfigLoader, MissingConfigException
Expand Down Expand Up @@ -192,6 +193,8 @@ def create( # pylint: disable=too-many-arguments

def _get_logging_config(self) -> Dict[str, Any]:
logging_config = self._get_config_loader()["logging"]
if isinstance(logging_config, omegaconf.DictConfig):
logging_config = OmegaConf.to_container(logging_config)
Comment on lines +196 to +197
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why logging config only works with it being a primitive container?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes actually two reasons:

  1. https://github.com/kedro-org/kedro/blob/main/kedro/framework/session/session.py#L197-L199 Needs a dict to properly convert the absolute paths
  2. this line expects a regular dict: https://github.com/kedro-org/kedro/blob/main/kedro/framework/project/__init__.py#L226

# turn relative paths in logging config into absolute path
# before initialising loggers
logging_config = _convert_paths_to_absolute_posix(
Expand Down
29 changes: 28 additions & 1 deletion tests/framework/session/test_session.py
View file
Open in desktop
Expand Up @@ -9,7 +9,7 @@
import yaml

from kedro import __version__ as kedro_version
from kedro.config import AbstractConfigLoader, ConfigLoader
from kedro.config import AbstractConfigLoader, ConfigLoader, OmegaConfLoader
from kedro.framework.context import KedroContext
from kedro.framework.project import (
ValidationError,
Expand Down Expand Up @@ -91,6 +91,16 @@ class MockSettings(_ProjectSettings):
return _mock_imported_settings_paths(mocker, MockSettings())


@pytest.fixture
def mock_settings_omega_config_loader_class(mocker):
class MockSettings(_ProjectSettings):
_CONFIG_LOADER_CLASS = _HasSharedParentClassValidator(
"CONFIG_LOADER_CLASS", default=lambda *_: OmegaConfLoader
)

return _mock_imported_settings_paths(mocker, MockSettings())


@pytest.fixture
def mock_settings_config_loader_args(mocker):
class MockSettings(_ProjectSettings):
Expand Down Expand Up @@ -889,3 +899,20 @@ def test_setup_logging_using_absolute_path(
).as_posix()
actual_log_filepath = call_args["handlers"]["info_file_handler"]["filename"]
assert actual_log_filepath == expected_log_filepath


@pytest.mark.usefixtures("mock_settings_omega_config_loader_class")
def test_setup_logging_using_omega_config_loader_class(
fake_project_with_logging_file_handler, mocker, mock_package_name
):
mocked_logging = mocker.patch("logging.config.dictConfig")
KedroSession.create(mock_package_name, fake_project_with_logging_file_handler)

mocked_logging.assert_called_once()
call_args = mocked_logging.call_args[0][0]

expected_log_filepath = (
fake_project_with_logging_file_handler / "logs" / "info.log"
).as_posix()
actual_log_filepath = call_args["handlers"]["info_file_handler"]["filename"]
assert actual_log_filepath == expected_log_filepath