Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Revamp Logging documentations in 0.19 #2665
Revamp Logging documentations in 0.19 #2665
Changes from 8 commits
dcc15e3
874f1a2
62abdfd
e6787d5
d97198c
1823dcb
7a91f11
8b4c0e5
0764cbf
4ecf24a
5eefee3
e27e334
c12ddc6
c234f11
29ffd05
517fc2f
1a04aeb
dad0012
f8ecc01
a383b4d
223e4a9
c4780dc
4400f0f
605b5e1
ad1fe98
73651b3
2465bdd
02584d8
78b2979
16557e6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this again, I realised I have no idea whatsoever what this means:
"The name of a logger corresponds to a key in the loggers section of the logging configuration file (e.g. kedro). See Python’s logging documentation for more information."
Does this have an relevance to the reader at this point or shall we remove it? Should it actually be a comment in the Kedro default_logging.yml file?
I think most readers at this point just want to know how to add logging to their code so won't need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is explaining the line
log = logging.getLogger(__name__)
, here is how you define thename
of a logger, and if you need configure it later what level of log you would like to see, you will do something like that. This is not kedro specific.If someone use a random name like
log = logging.getLogger("random_name")
, the above configuration will not have any effect. I think there is some value to educate user or provide a pointer to user, but we shouldn't try to explain too much about how Python logging work.There was a problem hiding this comment.
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 variableKEDRO_LOGGING_CONFIG
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hierarchy of this section feels wrong because there's no other way to customise logging now.
"How to customise Kedro logging" is basically the same as "Using KEDRO_LOGGING_CONFIG environment variable", and then "Show DEBUG level messages" is really just another example of a sort of customisation you want to make. All the other examples are currently given under "Advanced logging". So I think we should reorganise this a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we remove the sub-title "Using
KEDRO_LOGGING_CONFIG
environment variable" and just say that you need to use it for customisation. I'd like to keep the "How to customise Kedro logging" section heading but see what you're saying about the logic of the hierarchy. I've suggested the changes needed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have incorporate this comment partially. I still think it is valuable to put the DEBUG level example first, since this is a common thing to do compare to other niche advance use cases. I've reorder things a bit so it is more consistent.
There was a problem hiding this comment.
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 removedThere was a problem hiding this comment.
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
inconf
. Is it confusing to reference it asconf/logging.yml
or should I just reference it aslogging.yml
? @AhdraMeraliQBThere was a problem hiding this comment.
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 👍