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

Restructure data explorer #657

Merged
merged 6 commits into from
Nov 23, 2023
Merged

Conversation

PhilippeMoussalli
Copy link
Contributor

PR that restructures the data explorer to have multipages for the different functionalities. This provides a cleaner interface and better development of features in isolation.

Here is a schema to showcase the architecture of the new design:
image

Notes:

  • The session state of streamlit is used to save some configurations across different pages (selected component, partition, ...)
  • Some changes include additional logic to enable developing interactively outside of the docker image.
streamlit run main.py -- --base_path /home/philippe/Scripts/fondant-usecase-controlnet/data_dir

@PhilippeMoussalli
Copy link
Contributor Author

PhilippeMoussalli commented Nov 22, 2023

Overview of the new structure:

image

@PhilippeMoussalli PhilippeMoussalli linked an issue Nov 22, 2023 that may be closed by this pull request
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.

Thanks @PhilippeMoussalli!

The UI looks nice! One of the downsides of the multi-page app seems to be that the sidebar reloads every time, which doesn't feel that smooth. Is there a way around this?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for including this!

@RobbeSneyders
Copy link
Member

Not sure why this is happening, but the bottom of the dataset viewer gets cut off:
image

@PhilippeMoussalli
Copy link
Contributor Author

Thanks @PhilippeMoussalli!

The UI looks nice! One of the downsides of the multi-page app seems to be that the sidebar reloads every time, which doesn't feel that smooth. Is there a way around this?
.
Thanks! Unfortunately I don't a straight way around it. The multipage visualization is the only things that is consistent and does not flicker but the rest of the elements have to be reset on every page. They have the same issue in one of their examples: https://llm-examples.streamlit.app/.

Maybe we can move the options from the sidebar to the front page?

@RobbeSneyders
Copy link
Member

Maybe we can move the options from the sidebar to the front page?

That would be one option.

  • Downside is that these options are not as accessible, but I guess people won't switch pipeline often. They might want to switch run more regularly though.
  • Advantage would be that we can move the other options from the top of the page to the sidebar and win some space on the main view.

@PhilippeMoussalli
Copy link
Contributor Author

Maybe we can move the options from the sidebar to the front page?

That would be one option.

  • Downside is that these options are not as accessible, but I guess people won't switch pipeline often. They might want to switch run more regularly though.
  • Advantage would be that we can move the other options from the top of the page to the sidebar and win some space on the main view.

This is how the new design looks like. I managed to save some space by including some widgets on the same row. It should be faster now.

image

@PhilippeMoussalli
Copy link
Contributor Author

Not sure why this is happening, but the bottom of the dataset viewer gets cut off: image

I think the culprit is the columns_auto_resize_mode in the Aggrid when it's set to fit the column view

        # display the Ag Grid table
        AgGrid(
            dataframe_explorer,
            gridOptions=options_builder.build(),
            allow_unsafe_jscode=True,
            columns_auto_size_mode=ColumnsAutoSizeMode.FIT_ALL_COLUMNS_TO_VIEW,
        )

Seems like a newly introduced option but not sure how well optimized it is. Disabling it removes the cut off but then the columns are not fully expanded to the view:

image

Personally I would disable it, having some empty space in the column view is not a major inconvenience. Wdyt? Otherwise we can always submit an issue and opt for one of the two solutions temporarily


def create_common_interface(self):
"""Sets up the Streamlit app's main interface with common elements."""
add_logo("content/fondant_logo.png")
Copy link
Member

Choose a reason for hiding this comment

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

This file is not added in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I think you need to add streamlit_extras as well

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.

Thanks @PhilippeMoussalli!

I have to admit I didn't go through the code in detail. I mostly tested the app :)

@PhilippeMoussalli
Copy link
Contributor Author

Thanks @PhilippeMoussalli!

I have to admit I didn't go through the code in detail. I mostly tested the app :)

Yes I understand :) the diffs are large because of the restructuring but a lot of it is the same script with some small modifications. Next PRs should be easier to review

@PhilippeMoussalli PhilippeMoussalli merged commit 7ca280b into main Nov 23, 2023
6 checks passed
@PhilippeMoussalli PhilippeMoussalli deleted the restructure-data-explorer branch November 23, 2023 13:52
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.

Restructure explorer
2 participants