Skip to content

Conversation

@iamzili
Copy link
Contributor

@iamzili iamzili commented Aug 10, 2021

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

Copy link
Contributor

@daavoo daavoo 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 contribution @zili55 !

I have some doubts regarding the addition of CannotCreateLogDirError but they are not strictly related with this changes because the same doubts apply to the previous DvcLiveError

@codecov-commenter
Copy link

codecov-commenter commented Aug 10, 2021

Codecov Report

Merging #135 (55432e1) into master (7a4fd87) will increase coverage by 0.71%.
The diff coverage is 100.00%.

❗ Current head 55432e1 differs from pull request most recent head f9a33bd. Consider uploading reports for the commit f9a33bd to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #135      +/-   ##
==========================================
+ Coverage   89.28%   90.00%   +0.71%     
==========================================
  Files          12       12              
  Lines         308      310       +2     
==========================================
+ Hits          275      279       +4     
+ Misses         33       31       -2     
Impacted Files Coverage Ξ”
dvclive/error.py 94.44% <100.00%> (+2.13%) ⬆️
dvclive/metrics.py 96.15% <100.00%> (+1.76%) ⬆️

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 7a4fd87...f9a33bd. Read the comment docs.

Co-authored-by: David de la Iglesia Castro <daviddelaiglesiacastro@gmail.com>
Copy link
Contributor

@daavoo daavoo left a comment

Choose a reason for hiding this comment

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

Edited.

Copy link
Contributor

@daavoo daavoo left a comment

Choose a reason for hiding this comment

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

Thanks @zili55 ! Could you address:

#135 (review)

@iamzili
Copy link
Contributor Author

iamzili commented Aug 10, 2021

@daavoo P.R. to dvc.org created: treeverse/dvc.org#2707

and maybe from here https://dvc.org/doc/dvclive/api-reference/init#exceptions we should remove that exception

@daavoo
Copy link
Contributor

daavoo commented Aug 10, 2021

@daavoo P.R. to dvc.org created: iterative/dvc.org#2707

Nice

and maybe from here https://dvc.org/doc/dvclive/api-reference/init#exceptions we should remove that exception

Indeed, could you add that change in the same P.R.?

@daavoo daavoo merged commit 01c0513 into treeverse:master Aug 10, 2021
@daavoo daavoo linked an issue Aug 12, 2021 that may be closed by this pull request
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.

errors: get rid of DvcLiveError calls.

3 participants