Skip to content

Removes noisy and misleading print() in init#247

Closed
shcheklein wants to merge 1 commit into
mainfrom
remove-print
Closed

Removes noisy and misleading print() in init#247
shcheklein wants to merge 1 commit into
mainfrom
remove-print

Conversation

@shcheklein
Copy link
Copy Markdown
Contributor

  • It's not very expected from a lib to have log messages, especially using prints
  • Report will be saved at is printed in some cases but actual report is not created (e.g. if we use dvclive only as a tool to write plots, metrics, images)

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

@shcheklein shcheklein requested a review from daavoo April 29, 2022 06:46
Copy link
Copy Markdown
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.

I'm ok with this.

If I remember correctly, we added this to maintain parity with the previous behavior when the HTML was generated by DVC, where DVC will print this message at the beginning of running the stage.

@dberenbaum WDYT?

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 29, 2022

Codecov Report

Merging #247 (065c757) into main (07f3f7b) will decrease coverage by 0.04%.
The diff coverage is n/a.

❗ Current head 065c757 differs from pull request most recent head 36e17cf. Consider uploading reports for the commit 36e17cf to get more accurate results

@@            Coverage Diff             @@
##             main     #247      +/-   ##
==========================================
- Coverage   90.15%   90.11%   -0.05%     
==========================================
  Files          20       20              
  Lines         691      688       -3     
==========================================
- Hits          623      620       -3     
  Misses         68       68              
Impacted Files Coverage Δ
dvclive/live.py 96.94% <ø> (-0.07%) ⬇️

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 07f3f7b...36e17cf. Read the comment docs.

@dberenbaum
Copy link
Copy Markdown

  • It's not very expected from a lib to have log messages

IMO the primary scenario for dvclive is seeing a live-updated report of the model being trained. We have already made some compromises about the ease of seeing this report by not opening it by default (see the discussion in #243 (comment)). If we don't provide the user with the report path, it will be harder for users to even know the report is being generated and to know the path of the report. Do you have an alternate suggestion?

especially using prints

  • Report will be saved at is printed in some cases but actual report is not created (e.g. if we use dvclive only as a tool to write plots, metrics, images)

These seem like implementation details/bugs rather than reasons to remove the feature. Do you still think it should be removed if these are resolved @shcheklein?

@shcheklein
Copy link
Copy Markdown
Contributor Author

Do you have an alternate suggestion?

At least have a clear way to silence it? Usually it's fine for libs to have loggers, but there is a path to toggle them on / off.

The cleanest way to communicate back, would be probably some callback or some env variable that any consumer can read and decide what has to be printed.

Do you still think it should be removed if these are resolved @shcheklein?

If bugs are solved I don't feel it's that critical to remove this. You decide folks, I can probably see if I can rework this to solve the bug itself, not removing the whole thing.

@daavoo
Copy link
Copy Markdown
Contributor

daavoo commented May 3, 2022

At least have a clear way to silence it? Usually it's fine for libs to have loggers, but there is a path to toggle them on / off.

The cleanest way to communicate back, would be probably some callback or some env variable that any consumer can read and decide what has to be printed.

Added logger and env var in #250

Report will be saved at is printed in some cases but actual report is not created (e.g. if we use dvclive only as a tool to write plots, metrics, images)
These seem like implementation details/bugs rather than reasons to remove the feature.

The message is only logged if report has been passed to Live instantiation. This results in a lie when users don't call next_step or make_report.

However, showing the message when we know for a fact that report has been created is tricky as it can mess up with the ML frameworks progress bars, that's why we moved the message on DVC from the end of step to instantiation (#177)

@shcheklein
Copy link
Copy Markdown
Contributor Author

shcheklein commented May 3, 2022

Added logger and env var in https://github.com/iterative/dvclive/pull/250Added logger and env var in #250

👍

that's why we moved the message on DVC from the end of step to instantiation

Change the message then at least to be more explicit? "Report path (if generated): .... See more on report here ...."?

(btw, feel free to close this one)

@daavoo daavoo closed this May 3, 2022
@daavoo daavoo deleted the remove-print branch May 3, 2022 19:09
@shcheklein
Copy link
Copy Markdown
Contributor Author

Looks even uglier now, folks :(

INFO:dvclive:Report path (if generated): /Users/ivan/Projects/example-get-started/evaluation/report.html

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.

4 participants