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

Update mlflow with using resolve_tags #6746

Merged
merged 8 commits into from
Apr 8, 2021
Merged

Update mlflow with using resolve_tags #6746

merged 8 commits into from
Apr 8, 2021

Conversation

stllfe
Copy link
Contributor

@stllfe stllfe commented Mar 30, 2021 •
edited
Loading

Fixes #6745: adds additional info about the run, as in the native API. This message is also related to what I'd like to solve.

@stllfe
Update mlflow.py
b510247 
#6745 adds additional info about the run, as in the native API
@codecov
Copy link

codecov bot commented Mar 30, 2021 •
edited
Loading

Codecov Report

Merging #6746 (32aa976) into master (583fcf2) will decrease coverage by 9%.
The diff coverage is 89%.

@@           Coverage Diff            @@
##           master   #6746     +/-   ##
========================================
- Coverage      91%     83%     -9%     
========================================
  Files         192     192             
  Lines       12174   13174   +1000     
========================================
- Hits        11133   10869    -264     
- Misses       1041    2305   +1264

@stllfe
Update mlflow.py
0b8abbb 
trying to fix some backward compatibility issues with `resolve_tags`
@pep8speaks
Copy link

pep8speaks commented Mar 30, 2021 •
edited
Loading

Hello @stllfe! Thanks for updating this PR.

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

Comment last updated at 2021-04-05 06:15:37 UTC

stllfe added 2 commits March 31, 2021 00:11
@stllfe
wip on backward compatibility
57f4f44 
added a default for `getattr` in case the `registry` object exists, but has no proper attribute (weird case but who knows...)
@stllfe
fix pep
cd396d9
@stllfe
Copy link
Contributor Author

stllfe commented Mar 31, 2021

Any suggestions on code formatting and typing checks?

@stllfe stllfe requested a review from Borda March 31, 2021 14:52
@awaelchli awaelchli added logger Related to the Loggers feature Is an improvement or enhancement bug Something isn't working and removed feature Is an improvement or enhancement labels Mar 31, 2021
@Borda
Copy link
Member

Borda commented Mar 31, 2021

Any suggestions on code formatting and typing checks?

fixed, mind re-check whether it is what you want to get?

@Borda Borda added the ready PRs ready to be merged label Mar 31, 2021
@mergify mergify bot requested a review from a team March 31, 2021 21:52
stllfe
stllfe commented Apr 1, 2021
View reviewed changes
Copy link
Contributor Author

@stllfe stllfe left a comment •
edited
Loading

Choose a reason for hiding this comment

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

In my testing, it shows that everything works great with at least v1.0.0 and above of mlflow!

FYI, the part with resolve_tags works even below that version, but then some other places still do break. Say, in version 0.9.1 they had run.info.run_uuid as opposed to run.info.run_id in later versions, etc. However, there are more drastic changes in metrics logging API, so I guess it would be just easier to highlight somewhere that the lowest supported version is v1.0.0... In MLFlowLogger docs maybe?

pytorch_lightning/loggers/mlflow.py Outdated Show resolved Hide resolved
@kaushikb11 kaushikb11 removed the ready PRs ready to be merged label Apr 1, 2021
@Borda
Copy link
Member

Borda commented Apr 1, 2021

@stllfe mind check the failing test?

stllfe added 2 commits April 5, 2021 12:29
@stllfe
try fix failing tests
a4b254e 
removed the first if statement, so that `resolve_tags` would be defined either case
@stllfe
fix formatting
32aa976
@stllfe
Copy link
Contributor Author

stllfe commented Apr 5, 2021 •
edited
Loading

@Borda done! Removed that _MLFLOW_AVAILABLE check so that resolve_tags would be defined either way. However, it seems there still are some failed tests, idk why though... Can you check it? I'm not that familiar with pytest :(

@stllfe stllfe removed the request for review from a team April 5, 2021 07:47
@stllfe stllfe requested a review from Borda April 5, 2021 07:47
@carmocca
Copy link
Contributor

carmocca commented Apr 6, 2021

However, it seems there still are some failed tests, idk why though... Can you check it? I'm not that familiar with pytest :(

The failed tests are unrelated.

Can you also update the CHANGELOG?

Do you see a way to test this?

@carmocca carmocca added this to the 1.3 milestone Apr 6, 2021
@Borda Borda added the ready PRs ready to be merged label Apr 6, 2021
@mergify mergify bot requested a review from a team April 6, 2021 09:36
@Borda Borda changed the title Update mlflow.py Update mlflow with using resolve_tags Apr 6, 2021
@Borda Borda enabled auto-merge (squash) April 6, 2021 09:44
tchaton
tchaton approved these changes Apr 8, 2021
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.

LGTM !

@Borda Borda merged commit 3007872 into Lightning-AI:master Apr 8, 2021
@stllfe stllfe deleted the patch-2 branch April 8, 2021 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchaton tchaton tchaton approved these changes

@Borda Borda Borda approved these changes

@awaelchli awaelchli awaelchli approved these changes

Assignees
No one assigned
Labels
bug Something isn't working logger Related to the Loggers ready PRs ready to be merged
Projects
None yet
Milestone
pl:1.3
Development

Successfully merging this pull request may close these issues.

mlflow run context is not logged when using MLFlowLogger
7 participants
@stllfe @pep8speaks @Borda @carmocca @awaelchli @tchaton @kaushikb11