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

Batch updates to report elements in the App state #194

Merged
merged 8 commits into from Oct 4, 2019

Conversation

jrhone
Copy link
Contributor

@jrhone jrhone commented Sep 26, 2019

Batch every 10 ms of updates to elements in the App state.

  • upon receiving a new delta msg, wait 10ms for other delta msgs to arrive and add all of them to the state at once.
  • a new delta msg received after this batch will start it's own 10ms batch upon arrival.

frontend/src/App.jsx Outdated Show resolved Hide resolved
frontend/src/App.jsx Outdated Show resolved Hide resolved
@jrhone jrhone changed the title Batch report updates Batch updates to report elements in the App state Sep 28, 2019
@tvst
Copy link
Contributor

tvst commented Sep 29, 2019

I merged this into develop on my machine and then found a bug. Try this:

  1. Run the latest streamlit hello
  2. Select "animation example" on the left
  3. Press stop before it stops running
  4. Then streamlit run examples/graphviz_example.py

Expected: the hello demo gets replaced with graphviz_example
Actual: you get a merger of the two.

Copy link
Contributor

@tvst tvst left a comment

Choose a reason for hiding this comment

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

Ok, so I think your first solution was the cleanest after all 😁

We just need to tweak some of the names for clarity. Here's what I came up with:
tvst@43be303

@jrhone jrhone requested a review from tvst October 3, 2019 22:58
Copy link
Contributor

@tvst tvst left a comment

Choose a reason for hiding this comment

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

Looks familiar!

@@ -50,6 +50,8 @@ import "assets/css/theme.scss"
import "./App.scss"
import "assets/css/header.scss"

const ELEMENT_LIST_BUFFER_TIMEOUT_MS = 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment explaining what this is. Something like: "We update the elements list in the state in batches. This is how often we apply those batches."

@jrhone jrhone merged commit f7a3a6a into streamlit:develop Oct 4, 2019
tconkling added a commit to tconkling/streamlit that referenced this pull request Oct 7, 2019
* develop:
  Support cli parameters (streamlit#186)
  Update README.md
  Update README.md (streamlit#282)
  Bug fix: make Streamlit watch files in subfolders again (streamlit#265)
  fix st.code("") (streamlit#272)
  different jslint commands for circleci and not (streamlit#273)
  Batch updates to report elements in the App state (streamlit#194)
  Minor typo in pull_request_template.md (streamlit#262)
  Fix wrong quote used in the provided sample code (streamlit#267)
  Fix websocket port handling (streamlit#263)
  Closing sidebar when clicking outside (streamlit#223)
  Version 0.47.2 (streamlit#212)
  [docs] Changed value in Show progress section of docs/getting_started.md to fix error (Issue streamlit#234) (streamlit#235)
  Update README.md
  Remove checkbox from CLA in PR Template
  Update README.md
  Update README.md
  mapping_demo: catch urllib errors (streamlit#224)
tconkling added a commit to tconkling/streamlit that referenced this pull request Oct 8, 2019
* develop:
  Read config from ${CWD}/.streamlit/config.toml (streamlit#227)
  Fix unsafe_allow_html (streamlit#259)
  Support cli parameters (streamlit#186)
  Update README.md
  Update README.md (streamlit#282)
  Bug fix: make Streamlit watch files in subfolders again (streamlit#265)
  fix st.code("") (streamlit#272)
  different jslint commands for circleci and not (streamlit#273)
  Batch updates to report elements in the App state (streamlit#194)
  Minor typo in pull_request_template.md (streamlit#262)
  Fix wrong quote used in the provided sample code (streamlit#267)
  Fix websocket port handling (streamlit#263)
  Closing sidebar when clicking outside (streamlit#223)
  Version 0.47.2 (streamlit#212)
  [docs] Changed value in Show progress section of docs/getting_started.md to fix error (Issue streamlit#234) (streamlit#235)
  Update README.md
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

2 participants