Add manifest.json file tracking for automatic updates #46
Conversation
| if manifest_reader.has_changed: | ||
| st.rerun() |
There was a problem hiding this comment.
I'm not sure that I love the idea of restarting streamlit when we detect a change in the manifest. Is there some way we can move the reload of the manifest into the loop rather than just restarting the runtime?
There was a problem hiding this comment.
It looks like we already have a dependency on watchdog. Maybe we can set up a file listener to fire a callback on the event the manifest file is touched? Otherwise, can we just move the logic to rebuild the manifest into the while True: loop?
There was a problem hiding this comment.
Chris and I tried to get watchdog to work to monitor the user files and streamlit does not play nice with that at all. I'll check about rebuilding the manifest into the while True: loop, but I'm 99% sure the app still needs to rerun in order to have the manifest changes show up.
There was a problem hiding this comment.
Update: streamlit does need to rerun the app. We were trying to use watchdog to trigger a rerun of build_app, but streamlit behaves really strange when using watchdog for user files rather than their own files.
There was a problem hiding this comment.
Hmm... well that's unfortunate... I may halfheartedly look into it a bit myself when I get some time, but since since this refresh shouldn't be happening too often, I don't think its worth holding up approval over!!
tests/test_ManifestReader/test_manifestfilereader_load_manifest.py
Outdated
Show resolved
Hide resolved
tests/test_ManifestReader/test_manifestfilereader_load_manifest.py
Outdated
Show resolved
Hide resolved
tests/test_ManifestReader/test_manifestfilereader_get_manifest.py
Outdated
Show resolved
Hide resolved
tests/test_ManifestReader/test_manifestfilereader_has_changed.py
Outdated
Show resolved
Hide resolved
| if manifest_reader.has_changed: | ||
| st.rerun() |
There was a problem hiding this comment.
Hmm... well that's unfortunate... I may halfheartedly look into it a bit myself when I get some time, but since since this refresh shouldn't be happening too often, I don't think its worth holding up approval over!!
tests/test_ManifestReader/test_manifestfilereader_get_manifest.py
Outdated
Show resolved
Hide resolved
| @@ -64,9 +67,10 @@ def build_app(manifest_path: str) -> None: | |||
| ) | |||
|
|
|||
| while True: | |||
| if manifest_reader.has_changed: | |||
| st.rerun() | |||
| for v in to_update: | |||
| v.update() | |||
There was a problem hiding this comment.
I was thinking about what might happen if the manifest changes a few times (app re-runs immediately with no sleep, which might be undesirable) and noticed that v.update is called for all views, regardless of visibility.
Considering the way streamlit re-runs when we navigate around, could we get a performance boost by avoiding extraneous updates? I wonder if we should add a ticket for adding a similar concept to the views
for example: for v in [x for x in to_update if x.visible]: v.update()
There was a problem hiding this comment.
Love it! I'll work on a ticket for it
amandarichardsonn
added
type: feature
AlyssaCote
changed the title
In this PR I added an if check within the update loop that checks to see if there have been changes to the manifest file. If there have been changes, the app reruns.
We wanted to add this because new runs can be added to the manifest as the SmartSim experiment progresses, and the new changes to the manifest file were not being displayed in the dashboard without the user refreshing the page or interacting with widgets.