Skip to content

Conversation

@dberenbaum
Copy link

Drops the default log level from "info" to "warning" for dvc logging messages.

@dberenbaum dberenbaum requested a review from shcheklein May 25, 2023 00:40
self._dvc_repo = get_dvc_repo()

dvc_logger = logging.getLogger("dvc")
dvc_logger.setLevel(os.getenv(env.DVCLIVE_LOGLEVEL, "WARNING").upper())
Copy link
Member

Choose a reason for hiding this comment

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

do we still use logs to output UX/UI messages? (I thought we were migrating out of it)

any important messages (e.g. does it makes sense to still show this message once) that we might miss because of this?

(in YOLO I can for now do the log artifact once at the end)

Copy link
Author

Choose a reason for hiding this comment

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

do we still use logs to output UX/UI messages? (I thought we were migrating out of it)

At least in this case, we are still using logs.

Before:

>>> live.log_artifact("images")
100% Adding...|█████████████████████████████████████████████████|1/1 [00:00, 13.25file/s]

To track the changes with git, run:

        git add images.dvc

To enable auto staging, run:

        dvc config core.autostage true

After:

>>> live.log_artifact("images")
100% Adding...|█████████████████████████████████████████████████|1/1 [00:00, 12.77file/s]

In general, it's a good point. I would need to look deeper into when we use logs vs ui and if we need to also think about ui verbosity.

any important messages (e.g. does it makes sense to still show this message once) that we might miss because of this?

Again, I'd have to look deeper, but this change only drops the level from info to warn. Since info shouldn't have important messages, and warn is the default for python anyway, it theoretically should be fine.

My general feeling is that dvclive is too noisy overall and this change would be for the better, but I could also keep playing with it before merging. We may need to try for a bit to see if it feels like a good change.

Copy link
Contributor

Choose a reason for hiding this comment

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

any important messages (e.g. does it makes sense to still show this message once) that we might miss because of this?

If there is something really important we should be logging it from DVCLive logger

@dberenbaum dberenbaum self-assigned this May 25, 2023
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.

I am ok with the current change.

Still feels like we should later revisit the logging behavior from both DVC and DVCLive perspectives.

@daavoo daavoo merged commit ce68fe1 into main May 30, 2023
@daavoo daavoo deleted the suppress-dvc-logs branch May 30, 2023 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants