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

fix error when logging to progress bar with reserved name #5620

Merged
merged 13 commits into from
Jan 25, 2021
Merged

fix error when logging to progress bar with reserved name #5620

merged 13 commits into from
Jan 25, 2021

Conversation

awaelchli
Copy link
Contributor

@awaelchli awaelchli commented Jan 23, 2021 •
edited
Loading

What does this PR do?

Fixes #5299

The statement

self.log("loss", loss, prog_bar=True)

currently results in an error, because the loss is already logged to the progress bar by default.
Instead of an error message, we overwrite the progress bar dict entry by what the user logged and print a warning.

The warning was suggested by @edenlightning. Alternatively, we could also just overwrite the value without the warning because the user explicitly calls self.log anyway.

@awaelchli awaelchli added the bug Something isn't working label Jan 23, 2021
@awaelchli awaelchli added this to the 1.1.x milestone Jan 23, 2021
awaelchli
awaelchli commented Jan 23, 2021
View reviewed changes
tests/trainer/logging/test_logger_connector.py
@@ -425,3 +425,21 @@ def test_dataloader(self):
)
trainer.fit(model)
trainer.test(model, ckpt_path=None)


def test_logging_to_progress_bar_with_reserved_key(tmpdir):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tchaton @Borda Where should I put this test? I'm not familiar with how the logging tests are structured.

Copy link
Member

Choose a reason for hiding this comment

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

tests/trainer/logging_process ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this folder does not exist on master, only dev branch

Copy link
Member

Choose a reason for hiding this comment

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

oh, I see.... :]

Copy link
Contributor

@tchaton tchaton Jan 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe callback/test_progress ?

@awaelchli awaelchli mentioned this pull request Jan 23, 2021
Closed
@awaelchli awaelchli changed the title warn when logging to progress bar with reserved name fix error when logging to progress bar with reserved name Jan 23, 2021
@awaelchli awaelchli added the logging Related to the `LoggerConnector` and `log()` label Jan 23, 2021
@awaelchli awaelchli marked this pull request as ready for review January 23, 2021 01:04
@codecov
Copy link

codecov bot commented Jan 23, 2021 •
edited
Loading

Codecov Report

Merging #5620 (7ae943f) into master (052bc00) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #5620   +/-   ##
======================================
- Coverage      93%     93%   -0%     
======================================
  Files         135     135           
  Lines       10005   10011    +6     
======================================
- Hits         9351    9345    -6     
- Misses        654     666   +12

rohitgr7
rohitgr7 reviewed Jan 23, 2021
View reviewed changes
Copy link
Contributor

@rohitgr7 rohitgr7 left a comment

Choose a reason for hiding this comment

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

IMO at least adding a warning is better rather than silently updating it.

pytorch_lightning/trainer/properties.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/properties.py Outdated Show resolved Hide resolved
pytorch_lightning/trainer/properties.py Show resolved Hide resolved
pytorch_lightning/trainer/properties.py Outdated Show resolved Hide resolved
awaelchli and others added 2 commits January 23, 2021 22:44
@awaelchli @rohitgr7
suggestions from rohit
a900f81 
Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
@awaelchli
multiple values in message
5f26f11
@Borda Borda requested a review from rohitgr7 January 24, 2021 08:31
Borda
Borda approved these changes Jan 24, 2021
View reviewed changes
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

nice one :]

@Borda Borda added the ready PRs ready to be merged label Jan 24, 2021
@rohitgr7 rohitgr7 enabled auto-merge (squash) January 24, 2021 10:57
@rohitgr7 rohitgr7 merged commit b8d74ef into master Jan 25, 2021
@rohitgr7 rohitgr7 deleted the bugfix/duplicate_prog_bar branch January 25, 2021 12:57
@carmocca
Copy link
Contributor

@awaelchli can you move this test to logging_tests directory?. For the reasons described in #4451 (and #5452 (review))

We should be more mindful about where we place our tests

@awaelchli awaelchli mentioned this pull request Jan 27, 2021
Merged
@awaelchli
Copy link
Contributor Author

awaelchli commented Jan 27, 2021 •
edited
Loading

Here #5667

We should be more mindful about where we place our tests

I asked but was not fast enough to react before it was merged. I am mindful and I have suggested several times before to define a consistent testing file/folder structure.

tchaton pushed a commit that referenced this pull request Feb 3, 2021
@awaelchli @rohitgr7
@mergify @tchaton
fix error when logging to progress bar with reserved name (#5620)
fee3862 
* warn about duplicate metrics

* update changelog

* suggestions from rohit

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

* multiple values in message

* Apply suggestions from code review

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Borda pushed a commit that referenced this pull request Feb 4, 2021
@awaelchli @rohitgr7
@mergify @Borda
fix error when logging to progress bar with reserved name (#5620)
4509129 
* warn about duplicate metrics

* update changelog

* suggestions from rohit

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

* multiple values in message

* Apply suggestions from code review

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Borda pushed a commit that referenced this pull request Feb 4, 2021
@awaelchli @rohitgr7
@mergify @Borda
fix error when logging to progress bar with reserved name (#5620)
1bc063e 
* warn about duplicate metrics

* update changelog

* suggestions from rohit

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

* multiple values in message

* Apply suggestions from code review

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Borda pushed a commit that referenced this pull request Feb 4, 2021
@awaelchli @rohitgr7
@mergify @Borda
fix error when logging to progress bar with reserved name (#5620)
b58faad 
* warn about duplicate metrics

* update changelog

* suggestions from rohit

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

* multiple values in message

* Apply suggestions from code review

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Borda pushed a commit that referenced this pull request Feb 4, 2021
@awaelchli @rohitgr7
@mergify @Borda
fix error when logging to progress bar with reserved name (#5620)
b3b48c1 
* warn about duplicate metrics

* update changelog

* suggestions from rohit

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>

* multiple values in message

* Apply suggestions from code review

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rohitgr7 rohitgr7 rohitgr7 approved these changes

@Borda Borda Borda approved these changes

@tchaton tchaton tchaton approved these changes

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

Assignees
No one assigned
Labels
bug Something isn't working logging Related to the `LoggerConnector` and `log()` ready PRs ready to be merged
Projects
None yet
Milestone
pl:1.1.x
Development

Successfully merging this pull request may close these issues.

self.log("loss", loss, prog_bar=True) leads to an error
5 participants
@awaelchli @carmocca @Borda @tchaton @rohitgr7