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(sdk): wandb.Image breaks plt.imshow #7279

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

soumik12345
Copy link
Contributor

@soumik12345 soumik12345 commented Apr 2, 2024

Description

This PR attempts to fix the issue of wandb.Image breaking plt.imshow by replacing the logic of importing the PIL.Image module using wandb.util.get_module with a simple try-except check, that imports the PIL.Image module globally if installed and throws a wandb.Error otherwise.

Testing

The PR was tested using the following code snippet:

import wandb
import numpy as np


def plot_points(img):
    import matplotlib.pyplot as plt
    fig, ax = plt.subplots()
    plt.imshow(img)
    wandb.log({'Plot': plt})


wandb.init(project="test")
image = np.zeros((512, 512, 3))
wandb.log({"sample-image": wandb.Image(image)})
plot_points(image)

@soumik12345 soumik12345 requested a review from a team April 2, 2024 12:08
@soumik12345 soumik12345 self-assigned this Apr 2, 2024
@github-actions github-actions bot added cc-fix and removed cc-fix labels Apr 2, 2024
@soumik12345 soumik12345 changed the title fix: wandb.Image breaks plt.imshow fix(sdk): wandb.Image breaks plt.imshow Apr 2, 2024
@github-actions github-actions bot added cc-fix and removed cc-fix labels Apr 2, 2024
Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 76.24%. Comparing base (2dac381) to head (9e41e0d).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7279      +/-   ##
==========================================
- Coverage   78.73%   76.24%   -2.50%     
==========================================
  Files         508      508              
  Lines       54083    54092       +9     
==========================================
- Hits        42582    41241    -1341     
- Misses      11157    12503    +1346     
- Partials      344      348       +4     
Flag Coverage Δ
func 43.78% <38.88%> (-4.34%) ⬇️
system 64.70% <55.55%> (-0.01%) ⬇️
unit 57.86% <66.66%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
wandb/sdk/data_types/image.py 89.48% <66.66%> (-2.06%) ⬇️

... and 82 files with indirect coverage changes

Copy link
Contributor

@timoffex timoffex left a comment

Choose a reason for hiding this comment

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

Interesting! Why does the direct import work but not util.get_module()? Is there possibly a bug in get_module that should be fixed?

IMO the approach in this PR is clearer, but I don't have context on why we have get_module(). Based on its docs, there may be a thread-safety consideration. @kptkin - do you know?

@kptkin
Copy link
Contributor

kptkin commented Apr 3, 2024

Interesting! Why does the direct import work but not util.get_module()? Is there possibly a bug in get_module that should be fixed?

IMO the approach in this PR is clearer, but I don't have context on why we have get_module(). Based on its docs, there may be a thread-safety consideration. @kptkin - do you know?

Yeah, i don't know either, but if there is an issue with get_module it potentially could affect our entire code base, as we use this function all over. I asked @soumik12345 to investigate why get_module doesn't work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CLI]: wandb.Image breaks plt.imshow
3 participants