Skip to content

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

Merged
rohitgr7 merged 13 commits intomasterfrom
bugfix/duplicate_prog_bar
Jan 25, 2021
Merged

fix error when logging to progress bar with reserved name#5620
rohitgr7 merged 13 commits intomasterfrom
bugfix/duplicate_prog_bar

Conversation

@awaelchli
Copy link
Copy Markdown
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
Comment thread pytorch_lightning/trainer/properties.py
awaelchli
awaelchli commented Jan 23, 2021
View reviewed changes
Comment thread tests/trainer/logging/test_logger_connector.py
trainer.test(model, ckpt_path=None)


def test_logging_to_progress_bar_with_reserved_key(tmpdir):
Copy link
Copy Markdown
Contributor Author

@awaelchli awaelchli Jan 23, 2021

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
Copy Markdown
Collaborator

@Borda Borda Jan 23, 2021

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
Copy Markdown
Contributor Author

@awaelchli awaelchli Jan 24, 2021

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
Copy Markdown
Collaborator

@Borda Borda Jan 24, 2021

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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.

Comment thread pytorch_lightning/trainer/properties.py Outdated
Comment thread pytorch_lightning/trainer/properties.py Outdated
Comment thread pytorch_lightning/trainer/properties.py
Comment thread pytorch_lightning/trainer/properties.py Outdated
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
Copy Markdown
Collaborator

@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 to be merged PRs ready to be merged label Jan 24, 2021
rohitgr7
rohitgr7 reviewed Jan 24, 2021
View reviewed changes
Comment thread pytorch_lightning/trainer/properties.py Outdated
@rohitgr7 rohitgr7 enabled auto-merge (squash) January 24, 2021 10:57
rohitgr7
rohitgr7 reviewed Jan 24, 2021
View reviewed changes
Comment thread tests/trainer/logging/test_logger_connector.py Outdated
@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
Copy Markdown
Contributor

carmocca commented Jan 25, 2021

@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
Copy Markdown
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

@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

+1 more reviewer

@rohitgr7 rohitgr7 rohitgr7 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working logging Related to the `LoggerConnector` and `log()` ready to be merged 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