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

Streamlit UI Evaluation mode #920

Merged
merged 10 commits into from
Apr 22, 2021
Merged

Streamlit UI Evaluation mode #920

merged 10 commits into from
Apr 22, 2021

Conversation

PiffPaffM
Copy link
Contributor

PR for issue #864

@PiffPaffM PiffPaffM requested a review from oryx1729 March 25, 2021 12:28
@PiffPaffM PiffPaffM self-assigned this Mar 25, 2021
Copy link
Contributor

@Timoeller Timoeller left a comment

Choose a reason for hiding this comment

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

I would love to see the functionality in our streamlit app.
I made a comment to make it more adjustable from the outside.

Does it make sense to create tests about this feature and resulting feedback labels?

ui/webapp.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Timoeller Timoeller 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 adding the detailed user feedback section to our docs.

I made some more comments on the code.

There was also an open comment about file upload or attaching to an existing ES with pre indexed docs. Can we add some documentation on how to do that as well, please?

ui/README.md Outdated Show resolved Hide resolved
ui/README.md Outdated Show resolved Hide resolved
ui/README.md Show resolved Hide resolved
ui/st_state_patch.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Timoeller Timoeller left a comment

Choose a reason for hiding this comment

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

Some thoughts added on the st_state_patch.py

ui/st_state_patch.py Outdated Show resolved Hide resolved
@PiffPaffM
Copy link
Contributor Author

@Timoeller I went through your comments and added more information.
The gist st_state_patch.py still exists but the other implementation seems to be newer and is more likely the way going forward. Therefore, I changed that.

@Timoeller
Copy link
Contributor

Hey yes my bad. I saw these changes already briefly yesterday and was very happy about them. I did not find the time to properly check and approve the PR. Will do now.

Copy link
Contributor

@Timoeller Timoeller left a comment

Choose a reason for hiding this comment

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

Nice, thanks for adjusting the code.

About specifying EVAL_FILE=eval_labels_example.csv we can worry later. Because if somebody specifies a custom file that file still needs to somehow get into the docker at build time like you did with COPY eval_labels_example.csv /home/user/, correct?

@PiffPaffM
Copy link
Contributor Author

Nice, thanks for adjusting the code.

About specifying EVAL_FILE=eval_labels_example.csv we can worry later. Because if somebody specifies a custom file that file still needs to somehow get into the docker at build time like you did with COPY eval_labels_example.csv /home/user/, correct?

Yes, that is true. Let's find a better way in the future

@PiffPaffM PiffPaffM merged commit cf8a622 into master Apr 22, 2021
@PiffPaffM PiffPaffM deleted the streamlit_ui_eval branch April 22, 2021 15:30
@tholor
Copy link
Member

tholor commented Apr 22, 2021

About specifying EVAL_FILE=eval_labels_example.csv we can worry later. Because if somebody specifies a custom file that file still needs to somehow get into the docker at build time like you did with COPY eval_labels_example.csv /home/user/, correct?

Why not mounting the file into the container at runtime? We could add a volume in the docker-compose ...

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.

None yet

3 participants