-
Notifications
You must be signed in to change notification settings - Fork 37
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
fix: Improve safety of logging images with implicit formats #744
Conversation
I wonder if that's worth documenting? Sort of a minor change, but happy to put a PR into the docs site, too. |
Also, happy to write up the python docs in all the live.* functions if that's something that you think is useful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @natikgadzhi! Looks like it all works, but have some suggestions for refactoring it a bit.
src/dvclive/live.py
Outdated
if isinstance(val, (str, Path)): | ||
from PIL import Image as ImagePIL | ||
|
||
val = ImagePIL.open(val) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about handling errors here better? This would be a good place to suggest installing dvclive[image]
.
Summary
This pull request makes it so:
live.log_image
is called without image format in thename
, we will try to infer the format from the imagecould_log
instead of falling through toPIL.Image.save
Closes #743
Changes
@dberenbaum, I felt that both approaches need to be implemented together. Inferring the format is nice, but only works in some situations and not others. So for a situation where the format could not be inferred, I added an additional check that will fail early and won't attempt saving the image.
My Python must be not optimal. The changes are split into two commits, feature-wise.
could_log
I suggest we takename
andval
, not just val. This might be a bad idea if it's used elsewhere (not in this repository, I checked). It's not in public API, though.I'm not strongly attached to the
could_log
idea. In the end of the day, it's only saving us a bit of time, and clarifies the error, but it's not strictly required.TODO
❗ I have followed the Contributing to DVCLive
guide.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏