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

feature: adding streamlit interface to wise pizza #24

Closed
wants to merge 9 commits into from

Conversation

agusfigueroa-htg
Copy link

Context

This library is great, but opening a Jupyter notebook every time I want to run an analysis is a bit cumbersome. To solve this, I created a simple webapp interface using Streamlit. In there, one can easily upload a file, select the relevant columns (such as target, size and flag), have everything calculated and interact with the charts as on the Jupyter notebook.

In this version data can be only updated from the local machine, but with little effort a connection to the database can be added.

Changes

  • Add return_fig flag to the plotting and explaining functions, to improve compatibility with Streamlit
  • Create a functional Streamlit interface

Copy link

@julianteichgraber julianteichgraber left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR, that makes a lot of sense and I like this simple UI for wise-pizza! Great work!

I have left some comments, let me know if anything is unclear.

Just one additional idea:
Would be nice to only show columns with exactly two unique values in the split by selection column, but the current version will work as well of course

Comment on lines 9 to 18

root_path = os.path.realpath('../..')

# this assumes that all of the following files are checked in the same directory
sys.path.append(os.path.join(root_path,"wise-pizza"))

# create data-related directories
data_dir = os.path.realpath(os.path.join(root_path, 'wise-pizza/data'))
if not os.path.isdir(data_dir):
os.mkdir(data_dir)

Choose a reason for hiding this comment

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

would be nice to be able to launch the app from any folder (didn't work for me initially and I had to change the root_path)
You could add the streamlit folder to the wise-pizza folder and make the relevant changes to achieve this

Copy link
Author

Choose a reason for hiding this comment

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

Must have been solved, please try this again!

streamlit/streamlit_app.py Outdated Show resolved Hide resolved
streamlit/streamlit_app.py Outdated Show resolved Hide resolved
@agusfigueroa-htg
Copy link
Author

Amazing @julianteichgraber, thank you for your comments! Will address them here. Very happy to see this collab happening.
I am also working on a Docker Image for this, is it something you guys would be interested in?

@julianteichgraber
Copy link

julianteichgraber commented Nov 16, 2023

I am also working on a Docker Image for this, is it something you guys would be interested in?

Yes! Would also make sense to add a new CI test for the docker image then?

Also, feel free to extend the readme to mention the app / add an instruction on how to launch it

@agusfigueroa-htg
Copy link
Author

Hi @julianteichgraber, please take a look at the PR now and let me know how it went! I addressed all the points except for the CI one.

I am also working on a Docker Image for this, is it something you guys would be interested in?

Yes! Would also make sense to add a new CI test for the docker image then?

Also, feel free to extend the readme to mention the app / add an instruction on how to launch it

I assume you are thinking of building the docker image as a workflow before merging. Is that right? I got a bit lost on this one.

Copy link

@julianteichgraber julianteichgraber left a comment

Choose a reason for hiding this comment

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

Thanks for the changes and the readme update, looks good!!

@AlxdrPolyakov can you take a look and approve when you think it's ready? In particular, is the structure to have the streamlit app file in the root of the repo okay for you?

@AlxdrPolyakov
Copy link
Collaborator

Hey looks good, thank you, I will check in more detail and merge probably tomorrow after another PR

@AlxdrPolyakov
Copy link
Collaborator

Hey @agusfigueroa-htg I remember about it, still working on another PR :)

@AlxdrPolyakov
Copy link
Collaborator

Hey @agusfigueroa-htg, I checked the PR in more detail, thank you. Because of our last PR there are some conflicts, also I can't really continue work on this branch, so I can't make changes, that's why I created another branch, copied all your work and merged then. Added your github account to the streamlit file as a creator :)

I've just merged these updates and close PR.
But there are some important things to improve, if you want to do it yourself, go ahead! Otherwise I will do it later:

  • First of all I deleted return_config parameter from explain functions, we really don't need it because we have this parameter in the plot functions.
  • Current streamlit page is cool, but it is very limited, we can't really change any parameter. So the first thing to do is to add buttons for every parameter: solver, cluster_values, min_depth, max_depth, min_segments, max_segments, ... So we can change them on the streamlit page, not in code as we have now.
  • If we want to analyze explain_levels, we specify all columns. But then, if we want to switch to comparing time periods, we have to specify all parameters again. It is better if we specify the parameters only once.
  • As we have three different functions: explain_levels, explain_changes_in_totals, explain_changes_in_average, it's better to create one page for one function. Finally it's better to create four pages: data loading, explain_levels, explain_changes_in_totals, explain_changes_in_average.

Let me know if you want to do these tasks :) Otherwise I will do it.

And thanks a lot for this PR.

@agusfigueroa-htg
Copy link
Author

Hey @AlxdrPolyakov, thanks for the review and merging. The PR was of course limited but happy it gets us going!

I don't have time to work on this now so feel free to go ahead. I will give a heads up if I get to this before seeing another PR addressing your points already on its way.

Looking forward to the improved Streamlit app!

Cheers

@agusfigueroa-htg
Copy link
Author

Though unfortunately can't see my name neither under the streamlit app nor under the docker file 🤔

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