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 logged keys in mlflow logger #4412

Merged
merged 9 commits into from
Nov 10, 2020
Merged

fix logged keys in mlflow logger #4412

merged 9 commits into from
Nov 10, 2020

Conversation

dscarmo
Copy link
Contributor

@dscarmo dscarmo commented Oct 28, 2020 •
edited
Loading

What does this PR do?

A very simple change that fixes #4411, however requires discussion if removing the parenthesis is OK for all loggers.

Fixes #4411

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • 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? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

  • Is this pull request ready for review? (if not, please submit in draft mode)

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@awaelchli awaelchli added discussion In a discussion stage logger Related to the Loggers labels Oct 28, 2020
@codecov
Copy link

codecov bot commented Oct 28, 2020 •
edited
Loading

Codecov Report

Merging #4412 (e37f6d0) into master (11415fa) will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #4412   +/-   ##
======================================
  Coverage      93%     93%           
======================================
  Files         116     116           
  Lines        8854    8858    +4     
======================================
+ Hits         8244    8248    +4     
  Misses        610     610

@rohitgr7 rohitgr7 changed the title [#4411] fix gpu_log_memory with mlflow logger fix logged keys in mlflow logger Oct 28, 2020
tchaton
tchaton requested changes Nov 2, 2020
View reviewed changes
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.

Great catch ! Would you mind adding a warning and a test ? Possibility, the warning would be called only once, and we store mapping with the substitution.

pytorch_lightning/loggers/mlflow.py Outdated Show resolved Hide resolved
@edenlightning
Copy link
Contributor

@dscarmo friendly ping :) Mind making the changes so we can push this change?

@dscarmo
Copy link
Contributor Author

dscarmo commented Nov 10, 2020 •
edited
Loading

@dscarmo friendly ping :) Mind making the changes so we can push this change?

Just made the changes and tried to change a mlflow test to check for the original issue.
I was able to run tests locally, they failed on something not related but passed in the logger part.

I dont know if those changes require changing the Changelog or docs since we have the warning.

@Borda Borda added bug Something isn't working and removed discussion In a discussion stage labels Nov 10, 2020
tchaton
tchaton approved these changes Nov 10, 2020
View reviewed changes
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.

Thanks for the addition !

@rohitgr7 rohitgr7 merged commit 470e294 into Lightning-AI:master Nov 10, 2020
@edenlightning edenlightning added this to the 1.0.x milestone Nov 10, 2020
SeanNaren pushed a commit that referenced this pull request Nov 11, 2020
@dscarmo
fix logged keys in mlflow logger (#4412)
846a3d8 
* [#4411] fix gpu_log_memory with mlflow logger

* sanitize parenthesis instead of removing for all loggers

* apply regex for mlflow key sanitization

* replace ',' with '.' typo

* add single warning and test

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Co-authored-by: chaton <thomas@grid.ai>
(cherry picked from commit 470e294)
Borda pushed a commit that referenced this pull request Nov 11, 2020
@dscarmo @Borda
fix logged keys in mlflow logger (#4412)
f45f368 
* [#4411] fix gpu_log_memory with mlflow logger

* sanitize parenthesis instead of removing for all loggers

* apply regex for mlflow key sanitization

* replace ',' with '.' typo

* add single warning and test

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Co-authored-by: chaton <thomas@grid.ai>
(cherry picked from commit 470e294)
rohitgr7 added a commit that referenced this pull request Nov 21, 2020
@dscarmo @rohitgr7 @tchaton
fix logged keys in mlflow logger (#4412)
30af4fc 
* [#4411] fix gpu_log_memory with mlflow logger

* sanitize parenthesis instead of removing for all loggers

* apply regex for mlflow key sanitization

* replace ',' with '.' typo

* add single warning and test

Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Co-authored-by: chaton <thomas@grid.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awaelchli awaelchli awaelchli approved these changes

@Borda Borda Borda approved these changes

@tchaton tchaton tchaton approved these changes

@justusschock justusschock justusschock approved these changes

@rohitgr7 rohitgr7 rohitgr7 approved these changes

@ananyahjha93 ananyahjha93 Awaiting requested review from ananyahjha93

@nateraw nateraw Awaiting requested review from nateraw

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@teddykoker teddykoker Awaiting requested review from teddykoker

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

Assignees
No one assigned
Labels
bug Something isn't working logger Related to the Loggers
Projects
None yet
Milestone
pl:1.0.x
Development

Successfully merging this pull request may close these issues.

Using log_gpu_memory with MLFLow logger causes an exception.
7 participants
@dscarmo @edenlightning @awaelchli @Borda @tchaton @justusschock @rohitgr7