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

Decouple logging from ConfigLoader #2642

Merged
merged 31 commits into from
Jun 9, 2023
Merged

Decouple logging from ConfigLoader #2642

merged 31 commits into from
Jun 9, 2023

Conversation

noklam
Copy link
Contributor

@noklam noklam commented Jun 5, 2023

NOTE: Kedro datasets are moving from kedro.extras.datasets to a separate kedro-datasets package in
kedro-plugins repository. Any changes to the dataset implementations
should be done by opening a pull request in that repository.

Description

Fix #2426

related:

Note. Breaking, goes in 0.19.0.

Following #2281 we have decided that users will no longer be able to change logging configuration dependent on kedro run environment. This is a huge simplification and means that logging only gets configured once in the kedro process with LOGGING = _ProjectLogging().

There's a few things to be done here, but they all need to be done at the same time or it will be very confusing.

  • Remove KedroSession._setup_logging and KedroSession._get_logging_config 🥳
  • Remove everything to do with logging from all config loaders (e.g. in config_patterns and docstring examples)
  • Double check you've removed everything that needs removing by searching logging throughout the whole codebase. Note that configure_logging must stay since it's needed by the parallel runner
  • Make sure you explain in the release notes migration guide that a custom logging file is now independent of kedro run environment and only used if KEDRO_LOGGING_CONFIG is set to point to it
  • Related doc changes and example of logging.yml for advance users who may want to customise logging.

Development notes

Checklist

  • Read the contributing guidelines
  • 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

noklam added 5 commits June 5, 2023 12:05
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
@noklam noklam changed the base branch from main to develop June 5, 2023 14:53
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
@noklam noklam marked this pull request as ready for review June 6, 2023 12:58
@noklam noklam mentioned this pull request Jun 6, 2023
noklam added 2 commits June 6, 2023 14:10
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
@astrojuanlu
Copy link
Member

I see the target branch is develop ✔️ Does this fix all of gh-2426?

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
@noklam
Copy link
Contributor Author

noklam commented Jun 6, 2023

@astrojuanlu It's for 0.19.0 so it's in develop but not main. I planned to split the doc as a separate issue, but then I realised it's easier to have the doc in the same PR so I may just close #2645.

I haven't finished the doc change though.

noklam added 2 commits June 6, 2023 14:39
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
@noklam noklam marked this pull request as draft June 6, 2023 13:45
noklam added 3 commits June 6, 2023 16:34
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
@noklam noklam marked this pull request as ready for review June 7, 2023 10:57
@noklam
Copy link
Contributor Author

noklam commented Jun 7, 2023

Noted the develop branch is somehow outsync, I created this #2650 to manually fix it.

@noklam noklam requested review from antonymilne and removed request for astrojuanlu, idanov and yetudada June 7, 2023 11:01
Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

A few initial comments for now - I need to check through the codebase more carefully to see if there's anything else to do. But this looks great and is a huge improvement to kedro 💯

One thing I think is missing is that we can remove the logging.yml file from the project template now.

I think we should probably save the docs for a separate PR because it's a big rewrite, but up to you 🙂

noklam and others added 2 commits June 8, 2023 14:58
Co-authored-by: Antony Milne <49395058+antonymilne@users.noreply.github.com>
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
@noklam
Copy link
Contributor Author

noklam commented Jun 8, 2023

@antonymilne I will leave the big doc changes for the separate PR and keep it minimal in this PR.

noklam added 9 commits June 8, 2023 16:13
…o feat/divorce-logging

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
…o feat/divorce-logging

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
@noklam noklam requested a review from antonymilne June 8, 2023 17:24
@noklam
Copy link
Contributor Author

noklam commented Jun 8, 2023

I have addressed the comments and remove the logging template

Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

This is a huge improvement, thank you! 🙏 I would recommend picking up #2520 next so then we've got all the logging code changes complete and then write the docs after that.

When it comes to writing docs, we should be able to make the page MUCH simpler. Basically it would be something like:

  1. By default kedro, uses rich for logging. For the full setup, look at the default_logging.yml file
  2. If you want to change this then make your own logging.yml file. We recommend you put that file in conf. Then set KEDRO_CONFIG_LOGGING environment variable to point to it
  3. Show some of the things you might like to change and the code needed to do it:
    a. Turn off rich tracebacks
    b. Turn on file logging
    c. Show DEBUG level messages

No need to explain the difference between framework-side and logging-side configuration or any of that confusing stuff any more 🎉

@noklam noklam mentioned this pull request Jun 9, 2023
4 tasks
@noklam
Copy link
Contributor Author

noklam commented Jun 9, 2023

@antonymilne This sounds great! I think we may also need to revisit some deployment/databricks documentation too. I have open a ticket #2664

@@ -162,7 +156,7 @@ def create( # pylint: disable=too-many-arguments
conf_source=conf_source,
)

# have to explicitly type session_data otherwise mypy will complain
# have to explicitlyv type session_data otherwise mypy will complain
Copy link
Contributor

Choose a reason for hiding this comment

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

Small typo here

noklam added 5 commits June 9, 2023 11:01
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam noklam enabled auto-merge (squash) June 9, 2023 15:08
@noklam noklam merged commit 74f86fc into develop Jun 9, 2023
@noklam noklam deleted the feat/divorce-logging branch June 9, 2023 15:33
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.

Divorce config loader from logging.yml
4 participants