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.

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

Make logging.yml read by default #3831

Merged
merged 21 commits into from
May 8, 2024
Merged

Conversation

SajidAlamQB
Copy link
Contributor

@SajidAlamQB SajidAlamQB commented Apr 23, 2024

Description

Related to: #3801

Implement a fix to make sure the default file is read. KEDRO_LOGGING_CONFIG will take priority when it is provided.

Development notes

  • It will default to conf/logging.yml if KEDRO_LOGGING_CONFIG isn't set.
  • If conf/logging.yml does not exist, it falls back to default_logging.yml.

CONF_SOURCE

Investigated the possibility of adding custom CONF_SOURCE support during the logging setup, allowing the logging.yml to be dynamically set. Below are the results of this investigation:

Code changes:

class _ProjectLogging(UserDict):
    def __init__(self, settings: _ProjectSettings) -> None:
        """Initialise project logging. The path to logging configuration is given in
        environment variable KEDRO_LOGGING_CONFIG (defaults to conf/logging.yml)."""
        # Fetch CONF_SOURCE from settings
        conf_source = getattr(settings, 'CONF_SOURCE', 'conf')

        # Check if a user path is set in the environment variable
        user_logging_path = os.environ.get("KEDRO_LOGGING_CONFIG")

        # Check if the default logging configuration exists
        default_logging_path = Path(conf_source) / "logging.yml"
        if not default_logging_path.exists():
            default_logging_path = Path(__file__).parent / "default_logging.yml"

        # Use the user path if available, otherwise, use the default path
        if user_logging_path and Path(user_logging_path).exists():
            path = Path(user_logging_path)
        else:
            path = default_logging_path

        # Load and apply the logging configuration
        logging_config = Path(path).read_text(encoding="utf-8")
        self.configure(yaml.safe_load(logging_config))

PACKAGE_NAME = None

settings = _ProjectSettings()

LOGGING = _ProjectLogging(settings)

pipelines = _ProjectPipelines()

Explanation:

The above code attempted to fetch the CONF_SOURCE setting from the Kedro settings and use this setting to find the path of the logging config file, allowing the logging setup to change automatically depending on where the user specifies.

Issues:

During the implementation I ran into several problems mostly due to timings:

The dynamic fetching of settings led to circular imports because the logging module depended on settings, which in turn depended on other things that required logging. This dependency cycle caused runtime errors preventing the project from starting.

This approach also complicated the creation of the project. It was quite challengin to initialise logging before all configurations are loaded without causing import loops.

So for now the current approach of using the happy path for logging will only remain. For the users that need more dynamic logging they will have to manually configure logging. I've enhanced the documentation to outline this.

Developer Certificate of Origin

We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a Signed-off-by line in the commit message. See our wiki for guidance.

If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.

Checklist

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

SajidAlamQB and others added 4 commits April 23, 2024 14:54
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
@SajidAlamQB SajidAlamQB marked this pull request as ready for review April 30, 2024 09:08
@SajidAlamQB SajidAlamQB requested review from noklam, astrojuanlu and merelcht and removed request for merelcht April 30, 2024 09:08
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
@SajidAlamQB SajidAlamQB requested a review from yetudada as a code owner May 2, 2024 15:42
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Tested this locally and it works.

Wondering if we could add some logging message like

WARNING Using `conf/logging.yml` as logging configuration, you can change it by setting the KEDRO_LOGGING_CONFIG variable accordingly

to minimise surprises and clarify what's happening. What do you folks think?

Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Works:

❯ kedro run
Using `conf/logging.yml` as logging configuration. You can change this by setting the KEDRO_LOGGING_CONFIG environment variable accordingly.

I'll let others comment on the implementation. Leaving just one comment.

kedro/framework/project/__init__.py Outdated Show resolved Hide resolved
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Copy link
Contributor

@ankatiyar ankatiyar left a comment

Choose a reason for hiding this comment

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

Manual tested, works well! 👍🏾

@SajidAlamQB
Copy link
Contributor Author

Not sure why the windows test are getting a permission error. Anyone have any ideas?

Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com>
@SajidAlamQB SajidAlamQB requested a review from noklam May 8, 2024 11:10
@SajidAlamQB SajidAlamQB merged commit b82c681 into main May 8, 2024
41 checks passed
@SajidAlamQB SajidAlamQB deleted the feat/logging-read-by-default branch May 8, 2024 11:31
@noklam noklam linked an issue May 21, 2024 that may be closed by this pull request
@ankatiyar ankatiyar mentioned this pull request May 23, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make logging.yml read by default
4 participants