-
Notifications
You must be signed in to change notification settings - Fork 21
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
convert metric plots to plotly #359
Conversation
and this presents a security risk to end users that are sharing reports with each other, right?
Should we make an API end point for this?
have you done any experiments to determine if this is actually going to make a difference in page loading? |
Yes, this makes it more foolproof to ensure we're inserting a malicious script into a template.
If it would be useful beyond this single button, then sure we could add one. Otherwise following the js route seems simpler.
I haven't yet, but the intention here would be to first load the entire document, and then begin inserting metric plots into the correct divs. So we may not end up with a speedup in a completely rendered document, but shifting any long running rendering to the end of the line should improve things. |
solarforecastarbiter/datamodel.py
Outdated
def __check_plot_spec__(plot_spec): | ||
"""Ensure that the provided plot specification is valid JSON""" | ||
try: | ||
json.loads(plot_spec) |
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.
maybe https://python-jsonschema.readthedocs.io/en/stable/ instead?
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.
That's an interesting implementation of a 'JSON schema' parser. It doesn't seem to enforce the very opinionated specification proposed by JSON Schema .
From tinkering with this briefly, it looks like we'd still have to load the json string into a python dict and then call validate(instance=json_dict, schema={"type":"object"})
to use this effectively. It does look like it'll give us extra safety though, so that slight performance hit may be worth it?
@@ -313,7 +336,7 @@ def scatter(timeseries_value_cds, timeseries_meta_cds, units): | |||
return fig | |||
|
|||
|
|||
def construct_metrics_cds(metrics, rename=None): | |||
def construct_metrics_dataframe(metrics, rename=None): |
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.
Instead of deleting the existing code, we could create figures/plotly.py
and figures/bokeh.py
. Maybe not really needed but multiple plotting "backends" could be useful at some point.
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.
I think that's a really good idea. We could add logic to select the processing function based on the sfa reports version. Might require some minor tweaking to the bokeh handling, but I think that'd be a good idea.
Added the I've also added a script to insert the plotly plots when the standalone html file is generated. Otherwise on the dashboard, the metric_plots variable contains the json to create each plot and the appropriate divs for insertion. |
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.
I'm confused. I thought we'd put bokeh stuff in a bokeh module and plotly stuff in a plotly module.
docs/source/api.rst
Outdated
reports.figures.plotly_figures.timeseries_plots | ||
|
||
|
||
Retired functions for generating Bokeh metric plots. |
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.
Retired functions for generating Bokeh metric plots. | |
Functions for generating Bokeh metric plots. |
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.
still using time series
"""File containing necessary datamodel objects for creating bokeh metric | ||
plots. |
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.
"""File containing necessary datamodel objects for creating bokeh metric | |
plots. | |
"""File containing datamodel objects for creating bokeh metric plots. |
except (json.JSONDecodeError, ValidationError): | ||
raise ValueError('Figure spec must be a valid json object.') | ||
|
||
|
||
@dataclass(frozen=True) | ||
class ReportFigure(BaseModel): |
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.
PlotlyReportFigure
? Doesn't seem like a library-agnostic class.
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.
I think I misunderstood earlier, are you opposed to having an empty ReportFigure
parent class that gets sub classed by BokehReportFigure and PlotlyReportFigure? I think I mistook your opposition to one class with optional fields as being opposed to that as well.
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.
are you opposed to having an empty ReportFigure parent class that gets sub classed by BokehReportFigure and PlotlyReportFigure?
I am not opposed to that. It sounds reasonable. @alorenzo175 should comment before I waste more of your time.
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.
sub-classes are ok, but we should make sure they include a type
key so that it's easier to load into BokehReportFigure
or PlotlyReportFigure
when needed
solarforecastarbiter/datamodel.py
Outdated
svg: str | ||
figure_type: str | ||
category: str = '' | ||
metric: str = '' | ||
|
||
def __post_init__(self): | ||
__check_plot_spec__(self.spec) | ||
|
||
|
||
@dataclass(frozen=True) | ||
class RawReportPlots(BaseModel): |
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.
PlotlyRawReportPlots
? (as above)
This code is currently unreachable from the rest of the Solar Forecast Arbiter | ||
Core library. It may be used in place of the plotly_figures to generate bokeh |
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.
isn't the time series still used?
from bokeh.embed import components | ||
from bokeh.layouts import gridplot | ||
from bokeh.models import ColumnDataSource, Legend, CDSView, BooleanFilter | ||
from bokeh.models.ranges import Range1d | ||
from bokeh.plotting import figure | ||
from bokeh import palettes |
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.
no bokeh stuff in the plotly module
@lboeman seems that the 2nd is now done but not the first? |
@wholmgren Yes, and I found a bug in the plotting that I'm trying to rectify. You said that we don't necessarily need to display all months, or all days of the week for reports spanning only a few months or days correct? |
Hours plots should always have 0-24. Day of week should always have 7 days. I'm ok with other plots only showing periods for which there is actually data.
Seems like a good approach but if it's difficult then we could look at other options. |
# week of data. | ||
y_values = [plot_data[plot_data['index'] == day]['value'].iloc[0] | ||
if not plot_data[plot_data['index'] == day].empty | ||
else 0 for day in x_ticks] |
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.
can we use nan
instead of 0?
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.
will the same problem occur with hourly
if we exclude nighttime values?
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.
The hourly plots work correctly when dropping nighttime values. My test report excludes them. For some reason, plotly respects the x_range of an axis when provided as a series of numerical values, but not for things it decides are categorical.
solarforecastarbiter/reports/figures/tests/test_plotly_figures.py
Outdated
Show resolved
Hide resolved
solarforecastarbiter/reports/figures/tests/test_plotly_figures.py
Outdated
Show resolved
Hide resolved
I've opened an issue for adding pdf field to the datamodel. |
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.
looking pretty good. some minor things
solarforecastarbiter/datamodel.py
Outdated
spec: str | ||
svg: str | ||
figure_type: str | ||
figure_class: str = 'plotly' |
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.
Move this last and possibly make it a field(init=False)
so users can't set it
{% if plotly_version %} | ||
{# plotly js and the python plotly library do not have matching versions #} | ||
{% include "insert_plots.html" %} | ||
<script src="https://cdn.plot.ly/plotly-1.52.3.min.js"></script> |
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.
is there any reason to record plotly_version
in the report then?
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.
It seems like it might be useful to debug any issues that arise from an older report and newer core library. But maybe just recomputing is the answer there.
svg = fig.to_image(format='svg').decode('utf-8') | ||
except Exception: | ||
logger.error('Could not generate SVG for figure %s', | ||
getattr(fig, 'name', 'unnamed')) |
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.
I guess plotly figures don't have name? can you replace name with something that identifies the figure?
for mvalue in metric_result.values: | ||
new = { | ||
'name': metric_result.name, | ||
'abbrev': f(metric_result.name), |
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.
any idea why the abbreviate column is the same as name when I run the test report from the cli?
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.
I had neglected to pass the abbreviate function when generating the metrics_json
template var in template.py.
…gure title, pass abbreviation function when generating metrics_json object for csv download
…it, update report test
@alorenzo175 Think I've hit all your comments. I'm not really opposed to removing |
solarforecastarbiter/datamodel.py
Outdated
@@ -1255,10 +1257,9 @@ def from_dict(model, input_dict, raise_on_extra=False): | |||
dict_ = input_dict.copy() | |||
if model != ReportFigure: | |||
return super().from_dict(dict_, raise_on_extra) | |||
figure_class = dict_.get('figure_class') | |||
if figure_class == 'plotly': |
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.
eee, maybe making init false was a bad idea. I prefer using the figure class to determine this, so probably best to remove that, but keep the parameter at the end
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.
do we need to wait for the dashboard?
Dashboard PR SolarArbiter/solarforecastarbiter-dashboard#208 should work with this PR. Its tests should pass after this is merged. Any existing reports will need to be recomputed. I can take on the API issue to add a recompute endpoint so it'll be easier to roll this out into prod later if you're not already working on it. |
I'm not |
Closes #xxxx .docs/source/api.rst
for API changes.docs/source/whatsnew
for all changes. Includes link to the GitHub Issue with:issue:`num`
or this Pull Request with:pull:`num`
. Includes contributor name and/or GitHub username (link with:ghuser:`user`
).Bokeh's interactivity and loading are becoming an issue for larger reports. This converts all metrics plots to plotly. The motivations for this switch are as follows:
<script>
elements.The current code emulates the existing Bokeh plots closely, but there are a couple of things that need to be resolved before this is merged.