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

Revamp Logging documentations in 0.19 #2665

Merged
merged 30 commits into from
Jun 29, 2023
Merged

Revamp Logging documentations in 0.19 #2665

merged 30 commits into from
Jun 29, 2023

Conversation

noklam
Copy link
Contributor

@noklam noklam commented Jun 9, 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

#2664

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
update release note
dcc15e3 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam noklam linked an issue Jun 9, 2023 that may be closed by this pull request
Revamp logging documentation for 0.19 #2664
Closed
4 tasks
noklam added 4 commits June 9, 2023 15:17
@noklam
update logging conf/base mention and filebase logging
874f1a2 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam
Reorder the logging page
62abdfd 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam
reorder and update logging docs
e6787d5 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam
update docs
d97198c 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam noklam marked this pull request as ready for review June 14, 2023 14:52
@antonymilne
Copy link
Contributor

Just to understand, is this documentation meant to be for 0.18.x or for 0.19.0? 🤔 I was expecting the latter but the PR is directed to main?

@noklam noklam changed the base branch from main to develop June 16, 2023 00:43
AhdraMeraliQB
AhdraMeraliQB reviewed Jun 16, 2023
View reviewed changes
docs/source/logging/logging.md Outdated
```

### Show DEBUG level messages
To see your `DEBUG` level message, you can change the level of log messages that you wish to see in `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.

logging.yml is no longer included by default, before referencing it in the docs maybe we should include some section/instructions on how to overwrite the default configuration, including where to keep the file and how to use the new env variable KEDRO_LOGGING_CONFIG

docs/source/logging/logging.md Outdated
By changing the level value to `DEBUG` for the desired logger (e.g., <your_python_package>), you will start seeing `DEBUG` level messages in the log output.

## Customise Logging
In addition to the `rich` handler defined in Kedro's framework, the [project-side `conf/logging.yml`](https://github.com/kedro-org/kedro/blob/main/kedro/templates/project/%7B%7B%20cookiecutter.repo_name%20%7D%7D/conf/base/logging.yml) defines two further logging handlers:
Copy link
Contributor

Choose a reason for hiding this comment

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

conf/logging.yml will no longer exist - see: #2281 . Additionally console and info_file_handler are not included in the default logging setup

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 won't be in the default, we will provide it in a template instead.

docs/source/logging/logging.md Outdated
`KEDRO_LOGGING_CONFIG` is an optional environment variable that you can use to specify the path of your logging configuration file, overriding the default Kedro's `default_logging.yml`.

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:
In order to customise logging, you need to specify the path of your logging configuration file via setting the environment variable `KEDRO_LOGGING_CONFIG`, which overrides the default Kedro's `default_logging.yml` For example, you can set `KEDRO_LOGGING_CONFIG` as follows:
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to an earlier comment - perhaps we should move this section above any references to a project-side logging configuration.

docs/source/logging/logging.md Outdated
### Use plain console logging
File-based logging in Python projects aids troubleshooting and debugging. It offers better visibility into application's behavior and it's easy to search. However, it does not work well with read-only system such as [Databricks Repos](https://docs.databricks.com/repos/index.html).

To enable file-based logging, add `info_file_handler` in your `root` logger as follows in your `conf/logging.yml` as follow:
Copy link
Contributor

Choose a reason for hiding this comment

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

Instructions out of date as conf/logging.yml will be removed

Copy link
Contributor Author

@noklam noklam Jun 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It's not a must but we recommend user to keep their loggging.yml in conf. Is it confusing to reference it as conf/logging.yml or should I just reference it as logging.yml? @AhdraMeraliQB

Copy link
Contributor

@antonymilne antonymilne Jun 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think putting this file in conf is natural, but we should make it clear that users need to create that file themselves and that it doesn't already exist.

Edit: reading through the generated docs, I think this is clear already 👍

noklam and others added 3 commits June 19, 2023 12:00
@noklam @AhdraMeraliQB
Apply suggestions from code review
1823dcb 
Co-authored-by: Ahdra Merali <90615669+AhdraMeraliQB@users.noreply.github.com>
@noklam
add logging.yml template and reorder section
7a91f11 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam
Update logging and add logging.yml template
8b4c0e5 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
@noklam noklam requested a review from AhdraMeraliQB June 19, 2023 11:15
@noklam
Copy link
Contributor Author

noklam commented Jun 27, 2023 •
edited
Loading

@stichbury I made the following changes: a383b4d

Built doc: https://kedro--2665.org.readthedocs.build/en/2665/

  • Move the note section down a bit after the yml example is shown.
  • remove some default_logging.yml mentions, since this is not too important to the user. This is mentioned in a note section already.
  • Update the logging.yml example with <your_python_package> to make it consistent to the doc, otherwise if user copy and paste without it they may lost project's log.
  • The example is move up to "Show DEBUG level message" because user need the example to configure DEBUG level.

@noklam noklam requested review from stichbury, antonymilne and AhdraMeraliQB and removed request for AhdraMeraliQB June 27, 2023 16:59
@noklam noklam enabled auto-merge (squash) June 27, 2023 17:02
stichbury added 2 commits June 28, 2023 10:23
@stichbury
Revising logging to move page into index
c4780dc 
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
@stichbury
Few minor tweaks
4400f0f 
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
@stichbury
Copy link
Contributor

@noklam I made a pair of commits to this.

  1. To move the content into logging/index.md so that it simplifies the table of contents for the docs
  2. Couple of minor typo fixes.

I'll let it rebuild but there may be another commit needed if anything links to that docs page, since commit 1 will have broken the internal linking. I had problems rebuilding locally because this is develop...so I'll let it run through and fix from the build error.

Also, there's one issue I couldn't fix but doesn't seem quite right...I'll leave a comment in the file for you.

@stichbury
Fix broken internal linking
605b5e1 
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
@stichbury stichbury self-requested a review June 28, 2023 09:44
stichbury
stichbury approved these changes Jun 28, 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.

This is so much better and I am looking forward to getting it into main in due course.

AhdraMeraliQB
AhdraMeraliQB reviewed Jun 29, 2023
View reviewed changes
Copy link
Contributor

@AhdraMeraliQB AhdraMeraliQB 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! Haven't tested all the examples manually but everything reads well. Just one comment on the code block formatting:

docs/source/logging/index.md
root:
handlers: [rich]
```
</code>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure the code block should end here, the docs end up rendering as follows:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
Before you expand the template it looks pretty normal. But I agree it looks weird after it get expanded. I will add a short line there.

@noklam noklam requested a review from AhdraMeraliQB June 29, 2023 13:44
@noklam
Fix the wrong diff to show debug logging
16557e6 
Signed-off-by: Nok <nok.lam.chan@quantumblack.com>
AhdraMeraliQB
AhdraMeraliQB approved these changes Jun 29, 2023
View reviewed changes
Copy link
Contributor

@AhdraMeraliQB AhdraMeraliQB left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes! The tiniest nit but when the code block is expanded we have these weird lines above and below it:
image
Would be great to get rid of them if doable but they're nothing worth blocking the PR for imo

@noklam noklam merged commit 0be6381 into develop Jun 29, 2023
@noklam noklam deleted the noklam/revamp-logging-documentation-2664 branch June 29, 2023 15:40
@noklam noklam mentioned this pull request Jul 4, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stichbury stichbury stichbury approved these changes

@AhdraMeraliQB AhdraMeraliQB AhdraMeraliQB approved these changes

@antonymilne antonymilne Awaiting requested review from antonymilne

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

Successfully merging this pull request may close these issues.

Revamp logging documentation for 0.19
4 participants
@noklam @antonymilne @stichbury @AhdraMeraliQB