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

Extended docs with logging customisation at runtime #3968

Merged
merged 5 commits into from
Jun 27, 2024

Conversation

ElenaKhaustova
Copy link
Contributor

@ElenaKhaustova ElenaKhaustova commented Jun 26, 2024

Description

Solves #3918

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

Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
@ElenaKhaustova ElenaKhaustova marked this pull request as ready for review June 26, 2024 14:39
Copy link
Contributor

@datajoely datajoely left a comment

Choose a reason for hiding this comment

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

Some stylistic soft suggestions - I also wonder if it's worth listing out the other other class paths we may want to tell the user they can quieten 🤔

@@ -42,6 +42,17 @@ After setting the environment variable, any subsequent Kedro commands use the lo
If the `KEDRO_LOGGING_CONFIG` environment variable is not set, Kedro will use the [default logging configuration](https://github.com/kedro-org/kedro/blob/main/kedro/framework/project/default_logging.yml).
```

One can also customise logging at the runtime and redefine the logging configuration provided in the `logging.yml`.
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
One can also customise logging at the runtime and redefine the logging configuration provided in the `logging.yml`.
### Change the verbosity of specific parts of Kedro
One can also customise logging at the runtime and redefine the logging configuration provided in the `logging.yml`.

Copy link
Contributor

Choose a reason for hiding this comment

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

"You can" instead of "One can", maybe? Most of the docs are in second person I think

@@ -42,6 +42,17 @@ After setting the environment variable, any subsequent Kedro commands use the lo
If the `KEDRO_LOGGING_CONFIG` environment variable is not set, Kedro will use the [default logging configuration](https://github.com/kedro-org/kedro/blob/main/kedro/framework/project/default_logging.yml).
```

One can also customise logging at the runtime and redefine the logging configuration provided in the `logging.yml`.
In the example below we change a logging level to `WARNING` for the `kedro.io.data_catalog` component, so the setting is only applied to the `kedro.io.data_catalog` logger and for the rest of the components the default settings provided via `logging.yml` are remained unchanged.
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
In the example below we change a logging level to `WARNING` for the `kedro.io.data_catalog` component, so the setting is only applied to the `kedro.io.data_catalog` logger and for the rest of the components the default settings provided via `logging.yml` are remained unchanged.
In the example below we change a logging level to `WARNING` for the `kedro.io.data_catalog` component logger specifically, the logging for the rest of the components will remain unchanged.

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.

Apart from the vale suggestions and @datajoely's comment, LGTM!

Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
@ElenaKhaustova
Copy link
Contributor Author

Some stylistic soft suggestions - I also wonder if it's worth listing out the other other class paths we may want to tell the user they can quieten 🤔

I have applied the suggestions, thank you!

As for the other class paths, do you have any in mind that are worth adding or about which we have complaints? Overall, adding another path should be straightforward now even with this example.

@@ -42,6 +42,17 @@ After setting the environment variable, any subsequent Kedro commands use the lo
If the `KEDRO_LOGGING_CONFIG` environment variable is not set, Kedro will use the [default logging configuration](https://github.com/kedro-org/kedro/blob/main/kedro/framework/project/default_logging.yml).
```

One can also customise logging at the runtime and redefine the logging configuration provided in the `logging.yml`.
Copy link
Contributor

Choose a reason for hiding this comment

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

"You can" instead of "One can", maybe? Most of the docs are in second person I think

In the example below we change a logging level to `WARNING` for the `kedro.io.data_catalog` component logger specifically, the logging for the rest of the components will remain unchanged.
The same can be done for higher/lower-level components without affecting the top-level.

```python
Copy link
Contributor

Choose a reason for hiding this comment

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

This would go in the settings.py, right? Worth mentioning where to put this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good catch, I've added a clarification that this is relevant for the notebook environment. We cannot make it in settings.py since logging configuration from logging.yml is applied after settings.py configuration

def configure_project(package_name: str) -> None:
, so it will be redefined by what is provided in logging.yml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, the alternative is to customise logging.yml described in the neighbouring section.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, makes sense!

Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
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.

Some small suggestions, but otherwise looks good to me! 👍 Great way to resolve the original issue.

@@ -42,6 +42,21 @@ After setting the environment variable, any subsequent Kedro commands use the lo
If the `KEDRO_LOGGING_CONFIG` environment variable is not set, Kedro will use the [default logging configuration](https://github.com/kedro-org/kedro/blob/main/kedro/framework/project/default_logging.yml).
```

### Change the verbosity of specific parts of Kedro

You can also customise logging at the runtime and redefine the logging configuration provided in the `logging.yml` when using jupyter notebook.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
You can also customise logging at the runtime and redefine the logging configuration provided in the `logging.yml` when using jupyter notebook.
You can also customise logging at runtime and redefine the logging configuration provided in the `logging.yml` when using jupyter notebook.

### Change the verbosity of specific parts of Kedro

You can also customise logging at the runtime and redefine the logging configuration provided in the `logging.yml` when using jupyter notebook.
In the example below we change a logging level to `WARNING` for the `kedro.io.data_catalog` component logger specifically, the logging for the rest of the components will remain unchanged.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
In the example below we change a logging level to `WARNING` for the `kedro.io.data_catalog` component logger specifically, the logging for the rest of the components will remain unchanged.
The example below demonstrates how you can change the logging level from `INFO` to `WARNING` for the `kedro.io.data_catalog` component logger specifically, the logging for the rest of the components will remain unchanged.

Signed-off-by: Elena Khaustova <ymax70rus@gmail.com>
@ElenaKhaustova ElenaKhaustova merged commit a683cd3 into main Jun 27, 2024
10 checks passed
@ElenaKhaustova ElenaKhaustova deleted the docs/3918-disable-logging branch June 27, 2024 13:21
@merelcht merelcht linked an issue Jun 28, 2024 that may be closed by this pull request
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.

[DataCatalog]: Disable catalog.load() logging
5 participants