Workaround for deep import reloads not working #537
Conversation
d4275af to
64a79fa
Compare
64a79fa to
88d316b
Compare
|
Great stuff |
There was a problem hiding this comment.
Hey, @k4r1 ! Thanks for the contribution, it's a great work-around.
In the future, though, as requested in our pull request template and contributing guidelines, please check in before starting to code on a bug fix or feature request. In this specific case, @tconkling was already working on this issue, which means some effort has been duplicated in this process.
The code looks good to me, but it's possible I missed something. So I'll leave it to @tconkling to take a look before we move ahead with the merge (likely on Tuesday).
Also: please add a unit test for deep imports in lib/tests/streamlit/watcher/LocalSourcesWatcher_test.py
| # modules. | ||
| for wm in self._watched_modules.values(): | ||
| if wm.module_name is not None and wm.module_name in sys.modules: | ||
| del sys.modules[wm.module_name] |
There was a problem hiding this comment.
That's a really slick solution!
Since @tconkling was already looking at this issue, I'll let him take a look before we merge it in.
4966cb7 to
c4660ab
Compare
|
Hey @tconkling @tvst what's your release cycle like? Could we get a rough estimate for when this might make its way into a release? |
* develop: Fix cypress tests (streamlit#559) Extends capability to send config keys by param to "streamlit hello" command (streamlit#527) Explicitly use bash in Makefile (streamlit#556) Fix trailing whitespace (streamlit#562) Adding missing key parameter to text_input (streamlit#571) Create CONTRIBUTING.md Don't resize vega chart on update (streamlit#497) LaTeX support (streamlit#491) Workaround for deep import reloads not working (streamlit#537) Add dist-packages and site-packages to blacklist just in case. (streamlit#544)
|
Like @k4r1 I would really like to have this released as this would simplify my workflow at work and with awesome-streamlit.org. When can this be expected to be released? Thanks |
|
Sorry, I somehow missed this! This fix was released in version 0.50.x around 9 days ago. LMK if you're still seeing the issue! |
Issue: #366
Description:
This is a sketch for now, we may want to look at a more sophisticated fix. Although it does fix the linked issue, it does so in a somewhat scorched-earth manner.
Essentially what is happening is that if you have a module which is not directly imported by the base module (e.g.
app.pyimportsa.py,a.pyimportsb.py), then changes to that module will not be reloaded when a file change is detected.As far as I can understand, this is what is happening:
b.pybfromsys.modulesapp.pyapp.pyimportsa, which is already present insys.modules, and so we are donebin memory untilais reloadedThe ideal solution would be to unload both the modified module, as well as any modules that are importing it, but determining this tree of all possible paths for importing a given module may not be worth the effort.
Instead, what this PR does is simply unloads all watched modules. So in the scenario above,
agets unloaded as well whenbchanges, and so at the eval point both modules get reloaded, therefore reflecting the changes tobin the running app.Contribution License Agreement
By submiting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.