-
Notifications
You must be signed in to change notification settings - Fork 25
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
Explorer search #691
Explorer search #691
Conversation
Thanks @PhilippeMoussalli! Looks good. One small request. Can we make it so pressing enter in the search box executes a search? |
I'v included the change. Also fixed the graphviz visualization so it looks more aligned |
Thanks! Two more comments:
|
data_explorer/app/pages/dataset.py
Outdated
filter_cache_key = hashlib.md5( # nosec | ||
f"{search_field}_{search_value}_{exact_search}".encode(), | ||
).hexdigest() | ||
print(search_value) |
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.
Let's remove these print statements.
data_explorer/app/pages/dataset.py
Outdated
exact_search=exact_search, | ||
selected_fields=selected_fields, | ||
) | ||
print(df.compute()) |
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.
Let's remove these print statements.
There is no perfect way to fix this but It should look better now, I opted for this solution. They're not perfectly aligned but better than before
This should be fixed now, one other strange behavior I found is that toggling the exact match on/off triggers the search even if enter is not hit. That is because streamlit refreshes on every widget event change so in this case it will detect that a text is present in the search field (even if "Enter" is not submit) and trigger the search. There is no neat way to separate the two events. I reverted back to only allowing the search via the submit button. There is an open issue to remove the "Enter to apply" placeholder to avoid confusion so we probably should do this at some point but it's not there yet. |
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.
Ok, LGTM.
Only the images on the docs are broken.
|
||
![data explorer](../art/data_explorer/data_explorer.png) | ||
![data explorer](art/data_explorer/general_overview.png) |
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.
![data explorer](art/data_explorer/general_overview.png) | |
![data explorer](../art/data_explorer/general_overview.png) |
- Visualize long documents using a document viewer | ||
- Compare different pipeline runs (coming soon!) | ||
|
||
![data explorer](art/data_explorer/dataset_explorer.png) |
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.
![data explorer](art/data_explorer/dataset_explorer.png) | |
![data explorer](../art/data_explorer/dataset_explorer.png) |
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.
woops forgot to commit them. Added them, I think the existing references are valid
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.
Thanks @PhilippeMoussalli!
PR that adds search functionality to the explorer
other small improvements: