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

🎬 : Improved file uploader and camera input to call its on_change handler only when necessary #4270

Merged
merged 12 commits into from Jan 25, 2022

Conversation

kajarenc
Copy link
Collaborator

@kajarenc kajarenc commented Jan 11, 2022

We need that change to prevent on_change call for file_uploader on rerun.

📚 Context

Fix for #4256 .
When file uploaded to file_uploader, each rerun triggers on_change callback call.
This happens because we in session_state.py we check is old and new values are the same.
But since st.file_uploader returns a new UploadedFile object each time in deserialize method values are not equal.

I override __eq__ method for UploadedFile to check equality via underlying UploadedFileRecs ids

  • What kind of change does this PR introduce?

    • Bugfix
    • Feature
    • Refactoring
    • Other, please describe:

🧠 Description of Changes

  • Add bullet points summarizing your changes here

    • This is a breaking API change
    • This is a visible (user-facing) change

Revised:

Insert screenshot of your updated UI/code here

Current:

Insert screenshot of existing UI/code here

🧪 Testing Done

  • Screenshots included
  • Added/Updated unit tests
  • Added/Updated e2e tests

🌐 References

Does this depend on other work, documents, or tickets?

  • Issue: Closes #XXXX

Contribution License Agreement

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

…loadedFile instances with same FileRec.

We need that change to prevent `on_change` call for file_uploader on rerun.
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.

This LGTM, but I think we'll also want to check in with core platform to verify that doing this won't have any unintended consequences

Copy link
Contributor

@tconkling tconkling left a comment

Choose a reason for hiding this comment

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

This seems fine to me and I don't think it interferes with anything we're doing over here -

BUT, it'd be ideal if it had a test to prevent against regressions of the bug it's addressing. (Just something that asserts that the fileuploader callback isn't called when the file hasn't changed.)

Comment on lines 53 to 57
def __eq__(self, other):
if other is not None:
return self.id == other.id
return False

Copy link
Contributor

Choose a reason for hiding this comment

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

To be really correct, __eq__ should typecheck its argument (and we might as well also type-annotate the function)

def __eq__(self, other: object) -> bool:
  if not isinstance(other, UploadedFile):
    return NotImplemented
  return self.id == other.id

(This StackOverflow has more details about the weird "return NotImplemented" line: https://stackoverflow.com/questions/878943/why-return-notimplemented-instead-of-raising-notimplementederror)

@kajarenc kajarenc changed the title Add __eq__ method to UploadedFile, to be able to compare different Up… :ballot_box_with_ballot: Improved file uploader and camera input to call its on_change handler only when necessary Jan 24, 2022
@kajarenc kajarenc changed the title :ballot_box_with_ballot: Improved file uploader and camera input to call its on_change handler only when necessary 🎬 : Improved file uploader and camera input to call its on_change handler only when necessary Jan 24, 2022
@kajarenc kajarenc marked this pull request as ready for review January 25, 2022 20:14
@kajarenc kajarenc merged commit ef14d2f into streamlit:develop Jan 25, 2022
@Ericarru
Copy link

Ericarru commented Dec 27, 2023

Hi, maybe it's not the place but i am using streamlit 1.27 and I have checked for 1.28 and 1.29 too that "on change" function is executing every time the app is reloaded. Here snippet of my app:

import streamlit as st


def session_state_counter():
    if "counter" in st.session_state:
        st.session_state.counter = st.session_state.counter + 1
    else:
        st.session_state.counter = 1


file_raw = st.file_uploader(
    "Upload_file",
    on_change=session_state_counter(),  # on change does not work
)

st.button("Click me to test counter :)")

st.write(f"Counter: {st.session_state.counter}")

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

4 participants