Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Dec 22, 2020

Fixes #2497.

The issue isn't with st.number_input(), which wonderfully, correctly defaults to 0.0. The issue is st.slider() being passed a step of 0, which it's directly forwarding to BaseUI's Slider, which one can imagine is presumably is doing something like for (var curr = start; curr < end; curr += step) and dying in an infinite loop.

This PR:

  • Validates step to make sure it isn't 0
  • Adds a test for said validation

@ghost ghost self-requested a review December 22, 2020 00:31
@ghost ghost changed the title Add validation ensuring step cannot be 0. Add validation ensuring step cannot be 0 Dec 22, 2020
@ghost ghost changed the title Add validation ensuring step cannot be 0 Add validation to Slider ensuring step cannot be 0 Dec 22, 2020
@ghost ghost changed the title Add validation to Slider ensuring step cannot be 0 Add validation to st.slider ensuring step cannot be 0 Dec 22, 2020
@akrolsmir
Copy link
Contributor

Hm, looks like a flaky test, since File Uploader should be unrelated to any code changes here:

This test has been flaky before; we tried #2438 to fix it, but looks it may need more work...

For now, rerunning tests in CircleCI to unblock this PR

Copy link
Contributor

@akrolsmir akrolsmir left a comment

Choose a reason for hiding this comment

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

Looks good to me! Passes on PR-Preview

import streamlit as st

input_step = st.number_input("Step")
my_range = st.slider("Range", 20, 120, step=int(input_step))

We may want @asaini to take a look, for product approval on the string? It's very minor though.

@asaini
Copy link

asaini commented Dec 22, 2020

LGTM 👍

@ghost ghost merged commit fa554c7 into streamlit:develop Dec 22, 2020
@ghost ghost deleted the fix-slider-crash branch December 22, 2020 22:16
tconkling added a commit to tconkling/streamlit that referenced this pull request Jan 4, 2021
* develop:
  Bump vega from 5.17.1 to 5.17.3 in /frontend (streamlit#2541)
  Pick a random emoji on `st.set_page_config(emoji="random")` (streamlit#2020)
  Fix Jest warnings (streamlit#2523)
  Upgrade react-markdown (streamlit#2527)
  Upgrade react-hotkeys (streamlit#2525)
  Close streamlit#2495 (streamlit#2524)
  Remove unnecessary case statement (streamlit#2522)
  Bump @types/node from 12.19.9 to 14.14.16 in /frontend (streamlit#2526)
  Bump fetch-mock from 7.7.3 to 9.11.0 in /frontend (streamlit#2505)
  st.markdown now shows a link title (streamlit#2518)
  Bump @types/react-dom from 16.9.10 to 17.0.0 in /frontend (streamlit#2503)
  Fix caching list comprehensions (streamlit#2484)
  Add validation to st.slider ensuring `step` cannot be 0 (streamlit#2502)
  Ensure st.image works with UploadedFiles (streamlit#2512)
  Fix dataframe column sort (streamlit#2511)
  File uploader session check (streamlit#2498)
  Upgrade node-notifier to 8.0.1 or later (streamlit#2507)
  Fix st.number_input not using min_value as default value (streamlit#2499)
  Unblock Core patches, and add Marisa as a docs owner (streamlit#2501)
  Patch 0.73.1 (streamlit#2500)
This pull request was closed.
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.

Possible memory leak: passing int(None) to st.slider step value will crash streamlit

2 participants