Skip to content

Conversation

@carmocca
Copy link
Contributor

@carmocca carmocca commented May 20, 2021

What does this PR do?

Fixes #7183
Fixes #6203

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

@carmocca carmocca added this to the v1.4 milestone May 20, 2021
@carmocca carmocca self-assigned this May 20, 2021
@pep8speaks
Copy link

pep8speaks commented May 20, 2021

Hello @carmocca! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-06-10 11:43:50 UTC

@codecov
Copy link

codecov bot commented May 20, 2021

Codecov Report

Merging #7631 (4950821) into master (07b6923) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #7631   +/-   ##
======================================
- Coverage      93%     92%   -0%     
======================================
  Files         199     200    +1     
  Lines       12800   12801    +1     
======================================
- Hits        11855   11818   -37     
- Misses        945     983   +38     

@rohitgr7 rohitgr7 linked an issue May 21, 2021 that may be closed by this pull request
@tchaton tchaton marked this pull request as ready for review May 24, 2021 19:26
@carmocca carmocca added logging Related to the `LoggerConnector` and `log()` and removed design Includes a design discussion Important feature Is an improvement or enhancement labels Jun 9, 2021
@carmocca carmocca changed the title Logger connector refactor Clean-up after logger connector redesign Jun 9, 2021
@mergify mergify bot removed the has conflicts label Jun 9, 2021
@carmocca carmocca changed the title Clean-up after logger connector redesign Clean-up after logger connector redesign 1/2 Jun 9, 2021
@carmocca carmocca changed the title Clean-up after logger connector redesign 1/2 Clean-up after logger connector redesign 2/2 Jun 9, 2021
@mergify mergify bot removed the has conflicts label Jun 10, 2021
@carmocca carmocca added the ready PRs ready to be merged label Jun 10, 2021
@awaelchli awaelchli enabled auto-merge (squash) June 10, 2021 10:50
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

LGMT !

@mergify mergify bot removed the has conflicts label Jun 10, 2021
@awaelchli awaelchli merged commit b45a89a into master Jun 10, 2021
@awaelchli awaelchli deleted the refactor/logger-connector-poc branch June 10, 2021 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logging Related to the `LoggerConnector` and `log()` ready PRs ready to be merged refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LoggerConnector Refactor Flush logger connector metrics on Trainer state change

7 participants