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

Put framework logging into LOGGING and fix import resetting logging #1644

Merged
merged 28 commits into from
Jun 29, 2022

Conversation

antonymilne
Copy link
Contributor

@antonymilne antonymilne commented Jun 22, 2022

Description

Fixes #1558. Fixes Windows CI problem in #1592.

Also changed conf_logging to logging_config in a few places for consistency.

Development notes

This has been quite a journey, but after many, many different attempts at this I think I've now got a good solution 😬

Before:

Now:

  • LOGGING global variable still exists (there's no good way to get rid of it) but is now set to load framework default logging config initially and then gets overwritten with project logging config. The mechanism for this is a new _ProjectLogging class, which makes logging configuration much closer to how we configure other things in the same file
  • we no longer rely on the side-effect of importing kedro.config.default_logger to load setup framework logging config
  • in fact, we now no longer need this file. What it's doing fits in better in project.__init__.py anyway, so I've removed it
  • this is a very slight breaking change, because there's a small chance someone currently does import kedro.config.default_logger, which will no longer work. I checked on #kedro-users and discord and no one is knowingly doing this. The only case I can think of someone doing this would be from copy and pasting our current ipython extension script into a script of their own. This itself is quite unlikely, but if it is the case then it's comparable to very small breaking changes to non-essential workflows that we've put into minor releases before, so both Ivan and I are ok with this

Cause of Windows e2e test failures in #1592

This was very subtle and mysterious so let me document it here. The error was nothing to do with rich but actually a result of an import in a plugin!

  • test_plugin did from kedro.framework.cli.starters import ...
  • by Python's import mechanism, this imports in turn kedro.framework.cli ➡️ kedro.framework.main (due to __init__.py import) ➡️ import kedro.config.default_logger
  • this reset the logging mechanism partway through a spawned subprocess, which messed up the logging
    Now that there's no logging side-effects on file import this is not a problem.

Testing

Tested manually all the following cases:

  • kedro run with and without project logging.yml, with and without plugin with imports
  • kedro run --runner=ParallelRunner with and without project logging.yml, with and without plugin with imports
  • the above with a packaged project
  • kedro ipython and kedro jupyter lab
  • throwing exception inside a node

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

@antonymilne antonymilne changed the title fix/parallel-runner-logging Put framework logging into LOGGING and guard configure_logging Jun 22, 2022
Signed-off-by: Antony Milne <[email protected]>
kedro/config/config.py Outdated Show resolved Hide resolved
@antonymilne antonymilne changed the title Put framework logging into LOGGING and guard configure_logging Put framework logging into LOGGING and fix import resetting logging Jun 24, 2022
PACKAGE_NAME = None
LOGGING = None
LOGGING = _ProjectLogging()
Copy link
Contributor Author

@antonymilne antonymilne Jun 24, 2022

Choose a reason for hiding this comment

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

The major success here is that this now acts like pipelines and settings, and configure_logging is much more analogous to configure_project than it used to be.

Signed-off-by: Antony Milne <[email protected]>
Signed-off-by: Antony Milne <[email protected]>
Signed-off-by: Antony Milne <[email protected]>
Signed-off-by: Antony Milne <[email protected]>
Signed-off-by: Antony Milne <[email protected]>
@antonymilne antonymilne marked this pull request as ready for review June 24, 2022 21:34
# We suppress click here to hide tracebacks related to it conversely,
# kedro is not suppressed to show its tracebacks for easier debugging.
# sys.executable is used to get the kedro executable path to hide the top level traceback.
rich_traceback_install(
Copy link
Contributor

Choose a reason for hiding this comment

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

https://kedro.readthedocs.io/en/latest/search.html?q=rich&check_keywords=yes&area=default

We have the option to opt out from Rich logging, but do we have the option to opt out from the Rich traceback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, though not as easily as with configuring logging. You can do sys.excepthook = sys.__excepthook__ to change the traceback handler back to Python's default one. We haven't documented this anywhere yet because it's a bit obscure and not totally obvious where the best place to put that code would be, so I was planning to wait to see if any user asked it first 😅

Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

Awesome! Amazing work that fixes the very subtle bug and I like the more consistent _ProjectLogging

Copy link
Contributor

@SajidAlamQB SajidAlamQB left a comment

Choose a reason for hiding this comment

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

Awesome work, this looks perfect thank you! 🌟

Signed-off-by: Antony Milne <[email protected]>
RELEASE.md Outdated Show resolved Hide resolved
antonymilne and others added 3 commits June 29, 2022 14:51
Signed-off-by: Antony Milne <[email protected]>
…o fix/parallel-runner-logging

Signed-off-by: Antony Milne <[email protected]>

Signed-off-by: Antony Milne <[email protected]>
antonymilne and others added 6 commits June 29, 2022 14:53
Signed-off-by: Antony Milne <[email protected]>
…o fix/parallel-runner-logging

Signed-off-by: Antony Milne <[email protected]>

Signed-off-by: Antony Milne <[email protected]>
Signed-off-by: Antony Milne <[email protected]>
Signed-off-by: Antony Milne <[email protected]>
Signed-off-by: Antony Milne <[email protected]>
docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
@antonymilne antonymilne merged commit bcd59a5 into main Jun 29, 2022
@antonymilne antonymilne deleted the fix/parallel-runner-logging branch June 29, 2022 20:24
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.

Get parallel runner to recognise framework-side logging config
3 participants