Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

fix: removes all handlers from the root logger #4829

Merged
merged 1 commit into from
Dec 2, 2020
Merged

fix: removes all handlers from the root logger #4829

merged 1 commit into from
Dec 2, 2020

Conversation

tshu-w
Copy link
Contributor

@tshu-w tshu-w commented Dec 1, 2020

We should not mutate (insert/remove elements) the list you're currently iterating on. In fact, this causes handlers to not be completely deleted on my side.

For more discussion, see https://stackoverflow.com/questions/7484454/removing-handlers-from-pythons-logging-loggers

Copy link
Member

@epwalsh epwalsh left a comment

Choose a reason for hiding this comment

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

Thanks @tshu-w, great catch! Just need one small tweak to the CHANGELOG and then we can get this merged.

CHANGELOG.md Outdated
@@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- Added links to source code in docs.
- Fixed issue with GradientDescentTrainer when constructed with validation_data_loader==None and learning_rate_scheduler!=None.
- Fixed issue with removing all handlers in root logger.
Copy link
Member

Choose a reason for hiding this comment

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

This should be under the ### Fixed section (you may need to create this section if it doesn't exist under the ## Unreleased 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.

@epwalsh Does it look okay now? BTW, I've also moved the above fixed to this new section.

Copy link
Member

@epwalsh epwalsh left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@epwalsh epwalsh merged commit 458c4c2 into allenai:master Dec 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants