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

Bugfix data-explorer images #382

Merged
merged 8 commits into from
Aug 24, 2023
Merged

Bugfix data-explorer images #382

merged 8 commits into from
Aug 24, 2023

Conversation

PhilippeMoussalli
Copy link
Contributor

PR that fixes the issue with rendering images in the explorer. Images currently have type binary and not object. This was changed to render them properly.

Caveat: not all binary types will correspond to images so we will need to implement another logic for image detection but this fix should do for now.

Fixed another small bug related to errors in rendering the numerical explorer when no numeric columns are present

@ChristiaensBert
Copy link
Contributor

Looks good, are the images rendered decently fast? When did we make the change between binary and object?

import streamlit as st
from data import load_dataframe
from table import get_image_fields, get_numeric_fields
from widgets import (build_explorer_table, build_image_explorer,
build_numeric_analysis_plots,
build_numeric_analysis_table, build_sidebar)

dask.config.set({"dataframe.convert-string": False})
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it prevents the automatic conversion of bytes to string with Dask
#349

Copy link
Contributor

@ChristiaensBert ChristiaensBert left a comment

Choose a reason for hiding this comment

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

lgtm

@PhilippeMoussalli
Copy link
Contributor Author

Looks good, are the images rendered decently fast? When did we make the change between binary and object?

Only tested for 20 images but it was ok. Are there methods to optimize this?

Not sure if we every made the switch between the two types (might have happened a while ago). In general, this is the schema that is checked with the manifest:

https://github.com/ml6team/fondant/blob/50f3a97878ac81670ebe624039ff0fcec0542e4f/src/fondant/schema.py#L41C1-L42C1

@RobbeSneyders
Copy link
Member

RobbeSneyders commented Aug 23, 2023

I'm still getting a lot of issues just running fondant explore on this branch. Do I need to rebuild an image and update the version to get the latest code?

@PhilippeMoussalli
Copy link
Contributor Author

I'm still getting a lot of issues just running fondant explore on this branch. Do I need to rebuild an image and update the version to get the latest code?

I am using the fondant explore with the -tag flag with this commit hash 71214bb9de30cd4302d09af97acf117d4b0e9231 (latest from this PR)

It does require first building it with scripts/build_explorer.sh --tag 71214bb9de30cd4302d09af97acf117d4b0e9231

I assume this won't be an issue when it's merged to main (if it's part of the CI/CD)

Copy link
Member

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

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

Ok, it indeed works when rebuilding it myself. Thanks @PhilippeMoussalli!

@RobbeSneyders RobbeSneyders merged commit 64f7d44 into main Aug 24, 2023
5 checks passed
@RobbeSneyders RobbeSneyders deleted the bugfix-dataexplorer-images branch August 24, 2023 13:41
Hakimovich99 pushed a commit that referenced this pull request Oct 16, 2023
PR that fixes the issue with rendering images in the explorer. Images
currently have type `binary` and not `object`. This was changed to
render them properly.

Caveat: not all `binary` types will correspond to images so we will need
to implement another logic for image detection but this fix should do
for now.

Fixed another small bug related to errors in rendering the numerical
explorer when no numeric columns are present
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.

3 participants