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

Replace SessionState with Streamlit built-in #2006

Merged
merged 3 commits into from Jan 18, 2022

Conversation

yorickvanzweeden
Copy link
Contributor

@yorickvanzweeden yorickvanzweeden commented Jan 14, 2022

Proposed changes:

  • Replace SessionState with Streamlit's st.session_state

Why?

  1. The original gist (on which SessionState is based) recommends it
  2. SessionState contains an import (import streamlit.report_thread as ReportThread) that does not work in Streamlit 1.4.0 (released 2 days ago)
  3. SessionState only supports a single session. Using the demo from two different browsers results in unexpected behaviour. (Try running two queries from two tabs, one of the tabs will copy the question from the other tab)

Status (please check what you already did):

  • First draft (up for discussions & feedback)
  • Final code
  • Added tests
  • Updated documentation

@ZanSara ZanSara self-assigned this Jan 17, 2022
@ZanSara ZanSara added journey:first steps type:refactor Not necessarily visible to the users labels Jan 17, 2022
@ZanSara
Copy link
Contributor

ZanSara commented Jan 17, 2022

Hey @yorickvanzweeden, thank you for this contribution! Do you think there's any work left to be done or is it ready for review? If it's ready, I'm going to test it out and if it's all good we might merge it today. Let me know 🙂

@yorickvanzweeden
Copy link
Contributor Author

Hey @yorickvanzweeden, thank you for this contribution! Do you think there's any work left to be done or is it ready for review? If it's ready, I'm going to test it out and if it's all good we might merge it today. Let me know slightly_smiling_face

Hi @ZanSara , it should work already :)

@ZanSara
Copy link
Contributor

ZanSara commented Jan 17, 2022

Hey @yorickvanzweeden, I gave it a try very superficially and there is something wrong with the state management. Once I get answers for a question, if I click "Evaluation mode" all the answers disappear. See the attached GIF. Can you verify if it happens on your side as well?

demo-bug

My setup was pretty simple, I just checked out your branch, commented out the image directives in docker-compose.yml and then executed docker-compose up. Let me know if you can reproduce it.

@tholor tholor requested a review from ZanSara January 17, 2022 16:26
@yorickvanzweeden
Copy link
Contributor Author

Hi @ZanSara, I can confirm it on my side too. I will have a look at it

@yorickvanzweeden
Copy link
Contributor Author

@ZanSara, I was writing a default state (No results) on every Streamlit refresh. The bug is fixed and I rechecked the code. I don't expect any different behaviour, as all other changes in webapp.py are read/set statements.

@ZanSara
Copy link
Contributor

ZanSara commented Jan 18, 2022

Nice, I re-tested and it looks good now! Thank you very much for this contribution 🙂

@ZanSara ZanSara merged commit ea10d01 into deepset-ai:master Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:refactor Not necessarily visible to the users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants