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

Ensure logging.yml is optional and tidy logging config #1469

Closed
Tracked by #1461
antonymilne opened this issue Apr 22, 2022 · 4 comments · Fixed by #1543
Closed
Tracked by #1461

Ensure logging.yml is optional and tidy logging config #1469

antonymilne opened this issue Apr 22, 2022 · 4 comments · Fixed by #1543
Assignees

Comments

@antonymilne
Copy link
Contributor

antonymilne commented Apr 22, 2022

Must be completed after #1470.

Step 1

Delete (a) the file logging.yml and (b) the logs folder from all the following places:

  • kedro project template
  • kedro e2e test
  • all kedro starters

No longer doing the above; see #1461 (comment).

I think you'll get an exception like this:

kedro.config.abstract_config.MissingConfigException: No files found in ['/Users/antony_milne/kedro_stuff/projects/spaceflights/conf/base', '/Users/antony_milne/kedro_stuff/projects/spaceflights/conf/local'] matching the glob pattern(s): ['logging*', 'logging*/**', '**/logging*']

Catch this and emit as a debug level message instead. Write new tests/modify existing ones as needed.

kedro/config/logging.yml should remain! Don't delete that.

Step 2 - No longer doing this; see #1461 (comment).

Add the following to configure_logging after the logging.config line:

logging.getLogger(PACKAGE_NAME).setLevel(logging.INFO)

Add the following logger to kedro/config/logging.yml:

    # A logger for package_name is set at runtime in kedro.framework.project.__init__.
    # This is equivalent to the following entry.
    # package_name:
    #   level: INFO

Test to make sure this works as planned. Ask me to explain what this is all about.

Step 3

Tidy up tests. Look for all places we write a logging.yml in tests and remove them. local_logging_config appears to be the way we do this, but check any other references to logs to make sure they should still be there. local_local_config in tests/session/confest.py is a special case that should remain.

Step 4

Manual testing: check that everything works the same in terms of console and file logging when you do a kedro run. Make sure you test log messages coming from different places, e.g. something emitted from within the project.

@antonymilne antonymilne changed the title Ensure logging.yml is optional and remove it from all starters Ensure logging.yml is optional, delete it and logs folder Apr 22, 2022
@antonymilne antonymilne moved this to Todo in Kedro Framework Apr 22, 2022
@antonymilne antonymilne removed the status in Kedro Framework Apr 22, 2022
@datajoely
Copy link
Contributor

Do we want to drop the file handlers from lines 14:32?

info_file_handler:

@antonymilne antonymilne moved this to Todo in Kedro Framework Apr 22, 2022
@antonymilne
Copy link
Contributor Author

Maybe! See #1472.

@merelcht
Copy link
Member

merelcht commented May 9, 2022

Speak to @AntonyMilneQB for details on approaches.

@antonymilne
Copy link
Contributor Author

This issue has changed a bit: see #1461 (comment) for explanation.

@antonymilne antonymilne changed the title Ensure logging.yml is optional, delete it and logs folder Ensure logging.yml is optional May 16, 2022
@antonymilne antonymilne changed the title Ensure logging.yml is optional Ensure logging.yml is optional and tidy logging config May 16, 2022
@antonymilne antonymilne moved this from In Progress to In Review in Kedro Framework May 19, 2022
Repository owner moved this from In Review to Done in Kedro Framework May 20, 2022
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 a pull request may close this issue.

3 participants