-
Notifications
You must be signed in to change notification settings - Fork 4k
Improve module reloading #3001
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
Improve module reloading #3001
Conversation
vdonato
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a ton for the contribution @nrontsis! For the most part, this looks great to me -- I had a few comments that are just minor points.
We generally require two reviewers for community contributions, so on my end I'll bug some people to get another pair of eyes on this PR.
tconkling
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Thanks guys! I have now addressed @vdonato's feedback, I think 🚀 |
vdonato
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @nrontsis for both fixing #3000 and cleaning up the code in local_sources_watcher! 🙇
I think this is good to merge once @tconkling adds his official stamp of approval
|
Merged. Thanks, @nrontsis, this is great work. |
# By Vincent Donato (3) and Nikitas Rontsis (1) # Via GitHub * develop: Have theme hashes differentiate between being unset and the hash for null (#2993) Improve module reloading (#3001) Add tests to improve coverage around config options set via flag (#2991) Add snippets about CSS vars and minimum streamlit-component-lib versions (#3008) # Conflicts: # frontend/src/App.tsx
I ran into some trouble trying to call st.warning outside of code run by the ScriptRunner, which made me remember that some code changed in a recent PR (streamlit#3001) did just that. I double-checked that this code indeed doesn't work as calls that end up sending Delta protos rely on a ReportContext, which isn't available in the LocalSourcesWatcher. Since we can't present the warning in Streamlit itself (which would be ideal), I just downgraded this to be a warning log for now.
I ran into some trouble trying to call st.warning outside of code run by the ScriptRunner, which made me remember that some code changed in a recent PR (#3001) did just that. I double-checked that this code indeed doesn't work as calls that end up sending Delta protos rely on a ReportContext, which isn't available in the LocalSourcesWatcher. Since we can't present the warning in Streamlit itself (which would be ideal), I just downgraded this to be a warning log for now.
update_watched_modulesto reduce complexity