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.

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

DvcliveLoggerHook updates to work with DVC #1208

Merged
merged 16 commits into from
Dec 22, 2021

Conversation

daavoo
Copy link
Contributor

@daavoo daavoo commented Jul 20, 2021

Motivation

Update to use latest version of dvclive: https://github.com/iterative/dvclive/releases

@codecov
Copy link

codecov bot commented Jul 20, 2021

Codecov Report

Merging #1208 (5537a43) into master (7540cf7) will increase coverage by 0.53%.
The diff coverage is 80.00%.

❗ Current head 5537a43 differs from pull request most recent head 6de15af. Consider uploading reports for the commit 6de15af to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1208      +/-   ##
==========================================
+ Coverage   68.27%   68.80%   +0.53%     
==========================================
  Files         161      161              
  Lines       10742    10746       +4     
  Branches     1972     1973       +1     
==========================================
+ Hits         7334     7394      +60     
+ Misses       3023     2963      -60     
- Partials      385      389       +4     
Flag Coverage Δ
unittests 68.80% <80.00%> (+0.53%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mmcv/runner/hooks/logger/dvclive.py 80.76% <80.00%> (-5.60%) ⬇️
mmcv/ops/saconv.py 87.87% <0.00%> (+4.54%) ⬆️
mmcv/ops/deform_conv.py 72.26% <0.00%> (+10.21%) ⬆️
mmcv/ops/modulated_deform_conv.py 83.05% <0.00%> (+34.74%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7540cf7...6de15af. Read the comment docs.

@zhouzaida
Copy link
Collaborator

hi @daavoo , what is the default directory for saving log, and what will happen if the directory exists

@daavoo
Copy link
Contributor Author

daavoo commented Jul 21, 2021

hi @daavoo , what is the default directory for saving log, and what will happen if the directory exists

The default directory would be dvclive. And it it exists, it's contents will be removed (we are discussing about updating this behavior):

https://dvc.org/doc/dvclive/api-reference/init#parameters

When using this callback alongside DVC, dvclive will get those values from the ones specified in the stage:

https://dvc.org/doc/command-reference/stage/add#options

I removed the call to dvclive.init and the path argument as they are not strictly necessary and to prevent unexpected behaviors with default values (i.e. by default, dvclive will be used as output directory)

@daavoo
Copy link
Contributor Author

daavoo commented Jul 21, 2021

The default directory would be dvclive. And it it exists, it's contents will be removed (we are discussing about updating this behavior)

There is an open P.R. iterative/dvclive#122

@zhouzaida
Copy link
Collaborator

got it

@daavoo
Copy link
Contributor Author

daavoo commented Jul 28, 2021

Given the possibility of adding custom_imports to MMCV's config files, we would like to maintain this HOOK as part of the dvclive repository instead of here because dvclive is under development and we don't want to sature mmcv with P.R. for updating the hook.

Would it make sense to have the hook linked inside mmcv with something as follows?:

# mmcv/runner/hooks/logger/dvclive.py
from dvclive.mmcv import DVCLiveLoggerHook

Or should we just remove mmcv/runner/hooks/logger/dvclive.py?

@daavoo daavoo marked this pull request as draft July 28, 2021 09:10
@zhouzaida
Copy link
Collaborator

Given the possibility of adding custom_imports to MMCV's config files, we would like to maintain this HOOK as part of the dvclive repository instead of here because dvclive is under development and we don't want to sature mmcv with P.R. for updating the hook.

Would it make sense to have the hook linked inside mmcv with something as follows?:

# mmcv/runner/hooks/logger/dvclive.py
from dvclive.mmcv import DVCLiveLoggerHook

Or should we just remove mmcv/runner/hooks/logger/dvclive.py?

In fact, I prefer to keep the DVCLiveLoggerHook in the mmcv

@daavoo
Copy link
Contributor Author

daavoo commented Aug 6, 2021

Given the possibility of adding custom_imports to MMCV's config files, we would like to maintain this HOOK as part of the dvclive repository instead of here because dvclive is under development and we don't want to sature mmcv with P.R. for updating the hook.
Would it make sense to have the hook linked inside mmcv with something as follows?:

# mmcv/runner/hooks/logger/dvclive.py
from dvclive.mmcv import DVCLiveLoggerHook

Or should we just remove mmcv/runner/hooks/logger/dvclive.py?

In fact, I prefer to keep the DVCLiveLoggerHook in the mmcv

I will update the P.R. with the latest changes then

@daavoo daavoo marked this pull request as ready for review August 26, 2021 08:25
@zhouzaida
Copy link
Collaborator

hi @daavoo , please fix the CI

@daavoo
Copy link
Contributor Author

daavoo commented Sep 2, 2021

hi @daavoo , please fix the CI

It is passing now :)

@zhouzaida
Copy link
Collaborator

hi @daavoo , CI failed

@zhouzaida
Copy link
Collaborator

hi @daavoo , is there any progress?

@daavoo
Copy link
Contributor Author

daavoo commented Oct 18, 2021

hi @daavoo , is there any progress?

Sorry for late response. I will try to send updates today

@daavoo daavoo requested a review from zhouzaida October 18, 2021 20:49
@daavoo daavoo mentioned this pull request Oct 19, 2021
1 task
Co-authored-by: Zaida Zhou <58739961+zhouzaida@users.noreply.github.com>
@daavoo daavoo requested a review from zhouzaida November 9, 2021 20:01
"""

def __init__(self,
path,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean that we have deprecated the argument path? If so, should we raise a warning and handle that?

@ZwwWayne ZwwWayne merged commit ac92a11 into open-mmlab:master Dec 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants