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

Move to modules instead of classes for home page and common interface #717

Merged
merged 3 commits into from
Dec 11, 2023

Conversation

RobbeSneyders
Copy link
Member

This PR simplifies the data explorer implementation a bit by moving the home page and common interfaces into modules instead of a long hierarchy of classes with a singleton class at the top.

I think we should do the same for the dataset views, but that would increase the size of the PR, so I suggest to do it later in a separate one.

@@ -1,12 +1,9 @@
"""General configuration for the app."""

SESSION_STATE_VARIABLES = [
Copy link
Member Author

Choose a reason for hiding this comment

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

We should aim to get rid of these completely.

This is currently used to initialize all session state variables at the start of the application, to prevent errors when accessing non-initialized values. But this brings us in broken states anyway. We should make sure the flow is set up so values are only accessed after initialization, or non-initialization is handled in the accessing code.

Copy link
Member Author

Choose a reason for hiding this comment

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

This file was split into common.py and sidebar.py

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a fix for an issue raised by @Hakimovich99, where you cannot switch pipelines because the run selection would error.

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


class PipelineOverviewApp(MainInterface):
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to home.py

Copy link
Member Author

Choose a reason for hiding this comment

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

Extra logs since the docker compose logs don't show on Windows for Jan.

Copy link
Contributor

@Hakimovich99 Hakimovich99 left a comment

Choose a reason for hiding this comment

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

Thanks @RobbeSneyders! (I'm approving it but I've never deep dived in the explorer code, meaning I barely understand the changes. So my approval isn't that useful haha)

@RobbeSneyders RobbeSneyders merged commit b6e91d8 into main Dec 11, 2023
6 checks passed
@RobbeSneyders RobbeSneyders deleted the bugfix/data-explorer branch December 11, 2023 19:08
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.

2 participants