Add Database Telemetry page to SmartDashboard #38
Conversation
AlyssaCote
changed the title
AlyssaCote
changed the title
| pathlib.Path(__file__).parent.parent | ||
| / "tests/utils/manifest_files/manifesttest.json", |
There was a problem hiding this comment.
Not wrong per-say, but I feel kinda weird having our "default" path be something that (iirc, it might be worth checking me on this) we do not ship in the dashboard *.whl
There was a problem hiding this comment.
I'm starting to question if we even want this behavior? It makes it easy on me to test some quick manifest changes (which I selfishly love and use all the time), but I would hate for a user to try to launch the dashboard and get my weird testing manifest information somehow.
There was a problem hiding this comment.
Per offline discussion, it sounds like the consensus to to remove these default paths! I'll re-flag any I find in this review!
| try: | ||
| manifest = load_manifest(manifest_path) | ||
| st.session_state["manifest"] = manifest |
There was a problem hiding this comment.
Should this load_manifest call also be in a if "manifest" not in st.session_state block like in smartdashboard.pages.2_Database_Telemetry module?
There was a problem hiding this comment.
I'm not actually trying to access it in Experiment_Overview.py, so I left it out because I think forcing the manifest to be reloaded when going back to this page isn't a bad idea. However, now that the manifest file is tracked, maybe it should be added in so it's not reloaded when unnecessary? I'm a little torn.
| pathlib.Path(__file__).parent.parent.parent | ||
| / "tests/utils/manifest_files/manifesttest.json", |
There was a problem hiding this comment.
Similar concern here as with the other default path
| pathlib.Path(__file__).parent.parent | ||
| / "tests/utils/manifest_files/manifesttest.json", |
There was a problem hiding this comment.
Per offline discussion, it sounds like the consensus to to remove these default paths! I'll re-flag any I find in this review!
| pathlib.Path(__file__).parent.parent.parent | ||
| / "tests/utils/manifest_files/manifesttest.json", |
smartdashboard/views.py
Outdated
| self.chart: t.Optional[alt.Chart] = None | ||
|
|
||
| if self.telemetry: | ||
| self.telemetry_df = self.set_telemetry_df() |
There was a problem hiding this comment.
Not wrong, but tends to be a bit of a code smell in my book: We are setting an instance attr to the return value of an instance method. Can self.telemetry_df be an @property instead?
NOTE: no need to change this if this is done for performance/caching reasons, but if you could add a comment or something here explaining that that would be appreciated!
There was a problem hiding this comment.
It can't be a property because it's changing whenever there are additions to the graph dataframes. But I could set it like this:
if self.telemetry:
self.telemetry_df = self._load_data_update(skiprows=0)
AlyssaCote
added
the
type: feature
AlyssaCote
changed the title
MemoryView,ClientView, andOrchestratorSummaryVieware updating views housed inside ofTelemetryView, similar to the views withinOverviewViewfor theExperiment Overviewpage.Each new view has a corresponding
view_builderthat provides the Streamlit layouts.memory_file,client_file, andclient_count_filewere added to theShardschema.