-
Notifications
You must be signed in to change notification settings - Fork 190
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
Streamlit improvements #1060
Streamlit improvements #1060
Conversation
✅ Deploy Preview for dlt-hub-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few improvements in the UI before merging:
this is non-essential information. please move it to Load Info to the bottom. Also it would be cool to show the full pipeline state there but collapsed (hidden) by default
this is actually important. can we show it extended?
please do not show state view at all if state does not exist
order of the columns is for some reason random. it should be:
name | data_type | nullable
I'm happy we are able to test this
94cc7c8
to
6d26612
Compare
Added sorting of table by column names. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to refactor this app a little because it got messy.
- old app code is still there, why?
- old app code was a single page app with single entry and init logic also it forced an interface for displaying pages which was good.
- new app duplicates init code, pages are not self contained etc.
- why dashboard is not a page?
I like that pages are broken into blocks etc. but top level structure is gone in this PR
@sultaniman now I see
forget my (1) bullet point, didn't see that |
52ed0fc
to
f3dba23
Compare
9504dcb
to
309fbce
Compare
88acdea
to
0609daa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just two small thing in review. and one bigger:
when displaying schemas the order is not fixed! should be
name | data type | nullability
and is reverse
Also we're now manually enforcing the order of columns. |
7f8b32b
to
b8208ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! we should think how to eject this app from the lib so people can build their own stuff on top
def dummy_render(pipeline: dlt.Pipeline) -> None: | ||
pass | ||
|
||
old_args = sys.argv[:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just FYI you should patch this value in context manager so it restores at the end of test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it in the cli runner PR 🙂
This PR upgrades our streamlit app #459 which is launched when dlt pipeline PIPELINE_NAME show command is called.
It also includes better prompt to detect and install streamlit and additional packages if it is missing.
Also important thing for TODO is wait until
streamlit.AppTest
will merge the PR support for page transitions to add more thorough tests.This PR introduces a new argument for
pipeline
subcommand--hot-reload
which can be used for development purposes to allow hot reload of changing streamlit app code.Also to pass
pipelines_dir
to streamlit app I usedDLT_PIPELINES_DIR
environment variable.NOTE: I will remove old
streamlit_helper.py
once we merge this PR.Light color mode
Dark color mode
Error states
We need to show users some useful information for example if the destination does not support sql client or other capabilities.