Skip to content

Document integer size limit for number_input and slider #6724

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.

Sign up for GitHub

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

Jump to bottom
Merged
vdonato merged 3 commits into from
May 24, 2023
Merged

Document integer size limit for number_input and slider #6724

vdonato merged 3 commits into from
May 24, 2023
+12 −0

Conversation

@MathCatsAnd
Copy link
Contributor

@MathCatsAnd MathCatsAnd commented May 22, 2023 •
edited by snehankekre
Loading

Added note to docstrings for number_input and slider to clarify limitations for storing and returning large integers. Closes streamlit/docs#106

📚 Context

Programmatic limits exists for arguments in st.number_input and st.slider to prevent creating widgets having values larger in magnitude than +/- (1<<53) -1. These limits can be silently encountered if not passed as an argument upon creation. In that instance, incorrect integers will be stored and returned. Per a stale issue in streamlit/docs, a note has been added to the docstrings.

  • What kind of change does this PR introduce?
    • Documentation

Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@MathCatsAnd
Integer size limit for number_input and slider
617c323 
Added note to docstrings for `number_input` and `slider` to clarify limitations for storing and returning large integers
@vdonato vdonato requested a review from snehankekre May 22, 2023 22:56
@vdonato vdonato added the security-assessment-completed Security assessment has been completed for PR label May 22, 2023
vdonato
vdonato reviewed May 22, 2023
View reviewed changes
Copy link
Collaborator

@vdonato vdonato left a comment

Choose a reason for hiding this comment

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

Thanks @MathCatsAnd! This LGTM, but I'll let @snehankekre weigh in for an official review as our authority on docs 🙂

@MathCatsAnd
Copy link
Contributor Author

MathCatsAnd commented May 22, 2023

I just noticed a capitalization error from copy-pasting-editing. I will modify Javascript -> JavaScript to fix it. 🙂

@MathCatsAnd
Typesetting
818512a 
Changed Javascript to JavaScript for correct capitalization.
snehankekre
snehankekre approved these changes May 23, 2023
View reviewed changes
Copy link
Contributor

@snehankekre snehankekre left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Approving pending comment.

Thank you for fixing this long standing issue, @MathCatsAnd! 😄 Your note explains the problem, the cause, and the implications in a clear and organized way. nit: it could be slightly more concise. How about the following?

Integer values exceeding ``+/- (1<<53) - 1`` cannot be accurately stored or returned by the widget due to serialization constraints between the Python server and JavaScript client. Such numbers must be handled as floats, leading to a loss in precision.

Note: inline code in reStructuredText should be enclosed with two backquotes.

@snehankekre snehankekre changed the title Integer size limit for number_input and slider Document integer size limit for number_input and slider May 23, 2023
@MathCatsAnd
Copy link
Contributor Author

MathCatsAnd commented May 23, 2023

To Nit myself:

Since (1<<53) - 1 is a valid command but +/- isn't exactly, by any chance would it be more proper pull the +/- out like this:
Integer values exceeding +/- ``(1<<53) - 1`` cannot be ...

And would it be better to direct second sentence to "you" to try and be even clearer that the widget isn't going to convert automatically on the user's behalf? I had already tried to consider this, but I thought this might be even better. After all, the way things can go wrong go beyond just rounding error, so... You must handle such numbers as floats....

Integer values exceeding +/- ``(1<<53) - 1`` cannot be accurately stored or returned by the widget due to serialization constraints between the Python server and JavaScript client. You must handle such numbers as floats, leading to a loss in precision.

@snehankekre
Copy link
Contributor

snehankekre commented May 23, 2023

Yup, both of those look like improvements 👍

@MathCatsAnd
Docstring edit for succinctness -- integer limits
65f68ea 
Slightly reworded and trimmer warning about integer size limits in the docstrings of `slider` and `number_input`
@MathCatsAnd
Copy link
Contributor Author

MathCatsAnd commented May 23, 2023

Would it also be worth putting in an issue on streamlit/streamlit to request having None limits automatically interpretted as min_value=-2**53+1, max_value=2**53-1, when in integer mode? The widget behaves nicely, producing the correct warning to the user when manually set like that so that might help avoid giving some users a surprise:

image

@vdonato
Copy link
Collaborator

vdonato commented May 24, 2023

Would it also be worth putting in an issue on streamlit/streamlit to request having None limits automatically interpretted as min_value=-253+1, max_value=253-1, when in integer mode?

I think so! I'll go ahead and file an enhancement request for this. The one thing that may be slightly awkward about doing this by default is that the resulting numbers in the error message would probably look entirely arbitrary to everyone except for those that know what JS's min/max int values are. This is almost definitely still better than over/underflowing silently, but there may be a better product solution that we're not considering.

@vdonato vdonato mentioned this pull request May 24, 2023
Closed
@vdonato vdonato merged commit 951c1eb into streamlit:develop May 24, 2023
@MathCatsAnd MathCatsAnd deleted the widget-int-limits branch May 24, 2023 00:52
tconkling added a commit to tconkling/streamlit that referenced this pull request May 30, 2023
@tconkling
Merge branch 'develop' into tim/DevRequirementsNoProtobuf
31a39c0 
* develop: (22 commits)
  enzyme -> react-testing-library (Countdown, Modal, ProgressBar) (streamlit#6744)
  Remove console.log (streamlit#6753)
  Update `st.data_editor` and `st.dataframe` docstrings (streamlit#6752)
  Update `st.data_editor` session state format (streamlit#6711)
  Cypress flaky test fixes (streamlit#6743)
  Document integer size limit for number_input and slider (streamlit#6724)
  Change default theme hash on app init (streamlit#6729)
  Use .genericFonts instead of .fonts to fix bug with theme postMessage. (streamlit#6732)
  Improve startup performance by lazy loading some dependencies (streamlit#6531)
  Add support for Altair 5 (streamlit#6618)
  Update modals (streamlit#6688)
  Improve docstrings for `ttl` and `max_entries` (streamlit#6733)
  Improve `st.columns` docstring (streamlit#6727)
  Removed orphan line in dataframe docstring (streamlit#6734)
  Fix useIsOverflowing dependency array (streamlit#6731)
  Remove experimental from data editor (streamlit#6712)
  Clarify set_page_config docstring and exception message (streamlit#6594)
  Add a config option to disable warning for setting both a widget default and its key in session_state (streamlit#6640)
  Add column configuration API for `st.dataframe` and `st.data_editor` (streamlit#6598)
  Migrate datetime column formatting from date-fns to momentJS (streamlit#6702)
  ...
@sfc-gh-dmatthews sfc-gh-dmatthews mentioned this pull request Jul 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vdonato vdonato vdonato left review comments

+1 more reviewer

@snehankekre snehankekre snehankekre approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

security-assessment-completed Security assessment has been completed for PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Mention integer length limitations in number_input

3 participants

@MathCatsAnd @snehankekre @vdonato