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

fix: Improve safety of logging images with implicit formats #744

Merged
merged 6 commits into from
Feb 7, 2024

Conversation

natikgadzhi
Copy link
Contributor

Summary

This pull request makes it so:

  • If live.log_image is called without image format in the name, we will try to infer the format from the image
  • If we could not infer the format, we will raise an error message early in could_log instead of falling through to PIL.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.

  • I've also added unit tests for the behavior.
  • Behavior change: in could_log I suggest we take name and val, 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

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

@natikgadzhi natikgadzhi changed the title Improve safety of logging images with implicit formats fix: Improve safety of logging images with implicit formats Nov 30, 2023
@natikgadzhi
Copy link
Contributor Author

I wonder if that's worth documenting? Sort of a minor change, but happy to put a PR into the docs site, too.

@natikgadzhi
Copy link
Contributor Author

Also, happy to write up the python docs in all the live.* functions if that's something that you think is useful.

Copy link
Contributor

@dberenbaum dberenbaum left a 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.

Comment on lines 375 to 378
if isinstance(val, (str, Path)):
from PIL import Image as ImagePIL

val = ImagePIL.open(val)
Copy link
Contributor

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].

src/dvclive/live.py Outdated Show resolved Hide resolved
src/dvclive/live.py Outdated Show resolved Hide resolved
src/dvclive/live.py Outdated Show resolved Hide resolved
tests/plots/test_image.py Outdated Show resolved Hide resolved
@dberenbaum dberenbaum requested a review from a team January 4, 2024 22:28
@dberenbaum dberenbaum merged commit b1e6bf4 into main Feb 7, 2024
10 of 11 checks passed
@dberenbaum dberenbaum deleted the ng/bugfix/log-image-formats branch February 7, 2024 22:05
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.

log_image fails to save an image if the path was provided, but the name does not have an extension
3 participants