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

Add KEDRO_LOGGING_CONFIG env variable to specific where to read logging.yml #2535

Merged
merged 14 commits into from
Apr 28, 2023
Merged

Add KEDRO_LOGGING_CONFIG env variable to specific where to read logging.yml #2535

merged 14 commits into from
Apr 28, 2023

Conversation

noklam
Copy link
Contributor

@noklam noklam commented Apr 21, 2023 •
edited
Loading

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

Close #2206

Development notes

  • Add the new environment variable KEDRO_LOGGING_CONFIG
  • Add new test
  • Run test manually to make sure it picks up the logging file correctly.
  • doc change

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 noklam linked an issue Apr 21, 2023 that may be closed by this pull request
Make framework-side logging.yml user-specifiable #2206
Closed
@noklam noklam requested review from antonymilne and merelcht April 21, 2023 15:30
@noklam noklam marked this pull request as ready for review April 21, 2023 15:30
@noklam noklam requested a review from idanov as a code owner April 21, 2023 15:30
antonymilne
antonymilne approved these changes Apr 21, 2023
View reviewed changes
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.

Perfect, thank you! Would be nice to add this to the docs, but I'm also ok leaving that for when we've made some of the other changes and we can update the docs all in one go at the end.

RELEASE.md Outdated
@@ -12,6 +12,7 @@
# Upcoming Release 0.18.8

## Major features and improvements
* Added `KEDRO_LOGGING_CONFIG` for configuring logging.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Added `KEDRO_LOGGING_CONFIG` for configuring logging.
* Added `KEDRO_LOGGING_CONFIG` environment variable, which can be used to configure logging from the beginning of the `kedro` process.

@noklam noklam mentioned this pull request Apr 24, 2023
Closed
merelcht
merelcht reviewed Apr 25, 2023
View reviewed changes
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

This looks great 👍
I would be in favour of adding docs in this PR, since it's quite small and without docs the issue wouldn't really be complete yet.

@noklam noklam removed the request for review from idanov April 26, 2023 14:34
@noklam noklam requested review from merelcht and removed request for yetudada, astrojuanlu and stichbury April 26, 2023 14:55
merelcht
merelcht approved these changes Apr 26, 2023
View reviewed changes
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

Two minor comments on the docs, but otherwise this looks great! 👍

docs/source/logging/logging.md Outdated
@@ -17,6 +17,21 @@ The project-side logging configuration also ensures that [logs emitted from your

We now give some common examples of how you might like to change your project's logging configuration.

## Using `KEDRO_LOGGING_CONFIG` environment variable
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a top section like this or a subsection? It now looks like all next sections are part of this section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I'll make it a sub-section for now. It isn't really a "project-side" configuration anymore since it's global. But at the same time, if you have multiple project you will still likely set this environment variable per project via some .env file or IDE settings.

We can re-visit this later since we haven't removed logging from config_loader yet.

docs/source/logging/logging.md
To use this environment variable, set it to the path of your desired logging configuration file before running any Kedro commands. For example, if you have a logging configuration file located at `/path/to/logging.yml`, you can set `KEDRO_LOGGING_CONFIG` as follows:

```bash
export KEDRO_LOGGING_CONFIG=/path/to/logging.yml
Copy link
Member

Choose a reason for hiding this comment

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

Is this the same for Windows users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends, if you are running on bash then it should be the same. I do a quick search that we only mention export for a few other documentations as well. If we want to add mentions for how to make it work with CMD we can open a new ticket to do that.

Copy link
Member

Choose a reason for hiding this comment

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

It's fine if it's consistent with how we refer to other environment variables. 👍

@noklam noklam enabled auto-merge (squash) April 26, 2023 16:11
stichbury
stichbury approved these changes Apr 26, 2023
View reviewed changes
Copy link
Contributor

@stichbury stichbury left a comment

Choose a reason for hiding this comment

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

Looks great to me, thank you! 🌟

@noklam noklam merged commit 89504ec into main Apr 28, 2023
@noklam noklam deleted the 2206-make-framework-side-loggingyml-user-specifiable branch April 28, 2023 12:20
@noklam noklam mentioned this pull request Jun 9, 2023
Closed
4 tasks
@noklam noklam mentioned this pull request Apr 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merelcht merelcht merelcht approved these changes

@stichbury stichbury stichbury approved these changes

@antonymilne antonymilne antonymilne approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make framework-side logging.yml user-specifiable
4 participants
@noklam @stichbury @antonymilne @merelcht