Expire media files when ReportSession expires#1128
Merged
nthmost merged 40 commits intostreamlit:developfrom Mar 5, 2020
Merged
Expire media files when ReportSession expires#1128nthmost merged 40 commits intostreamlit:developfrom
nthmost merged 40 commits intostreamlit:developfrom
Conversation
nthmost
changed the title
tvst
reviewed
Feb 27, 2020
Contributor
tvst
left a comment
tvst
left a comment
There was a problem hiding this comment.
I think you can fix your python 3.5 integration test issue by replacing MediaFileManager's _get_session_id() with this version:
def _get_session_id():
"""Semantic wrapper to retrieve current ReportSession ID."""
ctx = get_report_ctx()
if ctx is None:
# This is only None when running "python myscript.py" rather than
# "streamlit run myscript.py". In which case the session ID doesn't
# matter and can just be a constant, as there's only ever "session".
return "dontcare"
else:
return ctx.report_session_id
tvst
reviewed
Feb 27, 2020
tvst
approved these changes
Mar 4, 2020
Contributor
tvst
left a comment
tvst
left a comment
There was a problem hiding this comment.
Approved! But please address the comments before merging.
|
|
||
|
|
||
| # Avoid circular dependencies in Python 2 | ||
| # Needed to avoid circular dependencies. |
Contributor
There was a problem hiding this comment.
So this is still needed in Python 3?
Contributor
Author
There was a problem hiding this comment.
When I take it out, the tests break. However, streamlit run x.py works without it...
tconkling
added a commit
to tconkling/streamlit
that referenced
this pull request
Mar 9, 2020
* develop: Unpin python-dateutil package version (streamlit#1153) Pypi nightly builds (streamlit#1171) Adding custom filterOptions function to SelectBox (streamlit#1182) Expire media files when ReportSession expires (streamlit#1128) 1159/use_container_width (streamlit#1174)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue: #415
Description: Our new MediaFileManager stores files for serving via HTTP, but doesn't have any limits on how long those files will be kept.
This PR introduces memory management for the MediaFileManager in the form of tracking which ReportSession is using which files. When all references to the files being used by different ReportSession IDs run out, those files are removed from the cache.
To accomplish this, some piping of the ReportSession ID is done in ScriptRunner and ReportThread using the report_session_id attribute / keyword.
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.