Integrate Plotly JavaScript for Job Viewer Tab #1729

aestoltm · 2023-05-19T19:36:32Z

Description

Timeseries Charts:

Timeseries charts are now rendered Plotly js charts instead of HighCharts

Analytic Charts:

Analytic charts are now rendered with Plotly js instead of Highcharts
Color brightness is slightly different than Highcharts.

Peer Charts (Gantt Charts):

No official support for gantt charts in Plotly js therefore rendered charts with Plotly shapes and added an underlying scatter plot due to shapes not supporting Plotly events and tooltips. Two other implementations I explored where using horizontal bar charts and box plot charts however the current implementation matches the highcharts version the best.

Timeseries Drilldown:

Adjusted plotly_click in ChartPanel.js to grab the nodeid of the trace's data point selected for drilldown redirect.
Adjusted how drilldown works in GanttChart.js to include the click event information in each data point and then when plotly_click fires action: 'show' is added to the click event info and is encoded for the drilldown redirect.

Export:

Updated selector in chrome-helper.js to the div in plotly_template.html
Updated layout for export chart to Plotly equivalent in chartImageResponse() in classes/Rest/Controller/WarehouseControllerProvider.php
Updated charting.php to grab file content from plotly_template.html
Created plotly_template.html. Uses layout and data given from charting.php to draw chart a div.

Known Issues/TODO

Fix text cutoff on error text for when there are more than four analytic charts. Plotly does not support any word wrapping, therefore, we can add a wordwrap library as this most likely will be needed for when updating other modules with Plotly js.
Optimize plotly_hover and plotly_unhover that mimics highcharts point hover events. Currently, not included with the PR but during testing the events crashed the webpage if hovering over charts with many overlapping traces. Note: this issue may be included in a future PR as it does not impact functionality

Motivation and Context

Currently, Highcharts requires a license to be purchased which is solved by using Plotly js an open-source library.

Tests performed

Added test cases in xdmod-supremm for changes made to Job Viewer
Performed GUI testing by visually comparing production and dev environment charts.

TODO

Add test cases for exporting Plotly charts
Add a button on Job Viewer Tab to redirect the reviewer to a random job with timeseries data.

Checklist:

The pull request description is suitable for a Changelog entry
The milestone is set correctly on the pull request
The appropriate labels have been added to the pull request

…peers and fixes rendering of envelope plots.

…also fixed some tooltip inconsistencies with Gantt Chart and Timeseries charts compared to production.

…er functionality.

…peers and fixes rendering of envelope plots.

…also fixed some tooltip inconsistencies with Gantt Chart and Timeseries charts compared to production.

…er functionality.

…error message format.

jpwhite4 · 2023-05-19T20:26:50Z

background_scripts/chrome-helper/chrome-helper.js

@@ -20,7 +20,7 @@ const args = require('yargs').argv;

    await page.goto('file://' + args['input-file']);

-    const innerHtml = await page.evaluate(() => document.querySelector('.highcharts-container').innerHTML);
+    const innerHtml = await page.evaluate(() => document.querySelector('.plotly').innerHTML);


You can't change this - it will break image export for the rest of XDMoD.

jpwhite4 · 2023-05-19T20:27:37Z

classes/Rest/Controllers/WarehouseControllerProvider.php

                ),

                'text' => $settings['show_title'] ? $data['schema']['description'] : null
            )
        );

-        if (strpos($data['schema']['units'], '%') !== false) {
+        /*if (strpos($data['schema']['units'], '%') !== false) {


Please don't add commented code - I'm sure the linter has already complained about this!

jpwhite4 · 2023-05-19T20:28:44Z

html/gui/js/modules/job_viewer/AnalyticChartPanel.js

+                    "y": 1.2
+                }
+            ]
+	    console.log("new annontation");


Please don't add console.log() statements (by all means use them when debugging - but not in the final code that gets reviewed).

jpwhite4 · 2023-05-19T22:11:12Z

html/gui/js/modules/job_viewer/ChartPanel.js

        }, // resize

        destroy: function () {
+/*


What is the new object lifecycle? Before we explicitly called destroy on highcharts charts to free up memory when the tab was closed. How does it work now in your code?

Plotly.purge(graphDiv) is supposed to be called here. I will add that to destroy.

This reverts commit f99f8ae.

This reverts commit a268968.

My comments were addressed

ryanrath

Just a couple of questions

html/gui/js/libraries/PlotlyUtilities.js

html/gui/js/modules/job_viewer/AnalyticChartPanel.js

jpwhite4 · 2023-06-30T14:29:43Z

classes/Rest/Controllers/WarehouseControllerProvider.php

-                break;
-            }
-        }
-
        $axisTitleFontSize = ($settings['font_size'] + 12) . 'px';


How come this code was left in but the rest of the setting transformation code was moved? Is there something different about the axis font sizes?

jpwhite4 · 2023-06-30T14:33:51Z

html/gui/js/modules/job_viewer/AnalyticChartPanel.js

-            this.chart = new Highcharts.Chart(this.chartOptions);
-
+        render: function () {
+            this.chart = Plotly.newPlot(this.id, [], this._DEFAULT_CONFIG.layout, this._DEFAULT_CONFIG.config);


Does it cause problems passing the same configuration objects to multiple plotly instances? I naively would have done a deep copy on the DEFAULT_CONFIG objects and then passed the copy into the plotly instance.

jpwhite4 · 2023-06-30T14:37:08Z

html/gui/js/modules/job_viewer/AnalyticChartPanel.js

+                    showarrow: false
+                }
+            ];
+            this._DEFAULT_CONFIG.layout.annotations = errorText;


I don't think it is a good idea to be editing the DEFAULT_CONFIG object. it can hardly be a portal-wide default if it changes accordiong to the state of an individual chart

jpwhite4 · 2023-06-30T14:37:37Z

html/gui/js/modules/job_viewer/AnalyticChartPanel.js

+        var trace = {};
+        if (data.error === '') {
+            var chartColor = this._getDataColor(data.value);
+            this._DEFAULT_CONFIG.layout.plot_bgcolor = chartColor.bg_color;


Same comment as above - not a good idea to edit the default that is shared among all charts.

…ghcharts comments/code.

…fault value inside function block.

ryanrath

LGTM. I"m good to merge once Joe has a look.

jpwhite4 · 2023-07-05T21:33:44Z

libraries/charting.php

 ) {
    $effectiveWidth = (int)($width*$scale);
    $effectiveHeight = (int)($height*$scale);

    $html_dir = __DIR__ . "/../html";
    $template = file_get_contents($html_dir . "/highchart_template.html");
+    if ($isPlotly){


Very minor point: if the chart is plotly then the code will open the file called highchart_template.html then allocate memory, then copy the contents of the file into memory, then delete the memory. Needless to say there is no point using cpu cycles to read the highchart_template file when in plotly mode.

Don't need to change this for this pull request. But should address this in the next set of code changes.

jpwhite4 and others added 21 commits February 2, 2023 16:37

Voltron Destroy

f21d893

Up

2b59a84

up

3c7c38e

Process of changing analytic charts over to plotly

d5bbcc4

More updates to analytic chart

0af6789

Fixed margin and background colors for analytic charts

cbc01ec

Updates to analytic chart formatting and error charts

1898fdc

Gantt chart Plotly converstion. Also includes drilldown fix for node/…

643858d

…peers and fixes rendering of envelope plots.

Removed Highcharts timezone dependency (now done through moment.js), …

119edf3

…also fixed some tooltip inconsistencies with Gantt Chart and Timeseries charts compared to production.

Basic support for hover events to better model highcharts orginal hov…

6b55daf

…er functionality.

Updates to chart responsiveness, axis formatting, and hover events.

2fef931

Gantt chart Plotly converstion. Also includes drilldown fix for node/…

833436c

…peers and fixes rendering of envelope plots.

Removed Highcharts timezone dependency (now done through moment.js), …

5fa6be1

…also fixed some tooltip inconsistencies with Gantt Chart and Timeseries charts compared to production.

Basic support for hover events to better model highcharts orginal hov…

eafc4e7

…er functionality.

Updates to chart responsiveness, axis formatting, and hover events.

61d05a9

Update export functionality for Plotly charts.

401e189

Merge changes from 10.5 into plotly branch.

b248363

Fix ChartPanel.js after merge.

6f24200

Update charts with one data point for a trace, update analytic chart …

4488665

…error message format.

Updates to export w/ plotly. Minor updates to timeseries charts.

bd0f587

Fix missing color from previous change.

6ce79dc

aestoltm added the Category:Job Viewer Job Viewer label May 19, 2023

aestoltm requested review from eiffel777, jpwhite4, ryanrath and connersaeli May 19, 2023 19:36

jpwhite4 reviewed May 19, 2023

View reviewed changes

aestoltm and others added 9 commits June 20, 2023 15:10

Revert "Minor adjustment to ES6 linting"

088b0f4

This reverts commit f99f8ae.

Revert "Adjust JS linter to not throw errors on ES6 syntax"

9819093

This reverts commit a268968.

Make JS linter happy

457c695

More syntax and styling changes

8f5e9db

More touch up to js syntax and styling

d1714d7

Linting issues with plotly utility function.

98a4e65

Should be last cleanup on linter.

d069b43

Merge branch 'xdmod10.5' into job_viewer_plotly

6b11547

Syntax and Styling adjustments

8482ab7

ryanrath reviewed Jun 23, 2023

View reviewed changes

Merge branch 'xdmod10.5' into job_viewer_plotly

58883cb

jpwhite4 reviewed Jun 30, 2023

View reviewed changes

aestoltm added 4 commits July 5, 2023 15:13

Address suggestions from PR reviewers. Also cleaned up some unused hi…

dffb587

…ghcharts comments/code.

Fixes for JS linter

db1ce72

Linter does not like default parameters, adjust approach to assign de…

c58440f

…fault value inside function block.

Updates to styling

ea1edfc

aestoltm marked this pull request as ready for review July 5, 2023 19:55

aestoltm requested review from jpwhite4, eiffel777 and ryanrath July 5, 2023 19:56

ryanrath reviewed Jul 5, 2023

View reviewed changes

jpwhite4 reviewed Jul 5, 2023

View reviewed changes

jpwhite4 approved these changes Jul 5, 2023

View reviewed changes

Merge branch 'xdmod10.5' into job_viewer_plotly

2e6b0ed

aestoltm merged commit ab1f25f into ubccr:xdmod10.5 Jul 5, 2023

aestoltm deleted the job_viewer_plotly branch May 16, 2024 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate Plotly JavaScript for Job Viewer Tab #1729

Integrate Plotly JavaScript for Job Viewer Tab #1729

aestoltm commented May 19, 2023 •

edited

Loading

jpwhite4 May 19, 2023

jpwhite4 May 19, 2023

jpwhite4 May 19, 2023

jpwhite4 May 19, 2023

aestoltm May 19, 2023

ryanrath left a comment

jpwhite4 Jun 30, 2023

jpwhite4 Jun 30, 2023

jpwhite4 Jun 30, 2023

jpwhite4 Jun 30, 2023

ryanrath left a comment

jpwhite4 Jul 5, 2023

Integrate Plotly JavaScript for Job Viewer Tab #1729

Integrate Plotly JavaScript for Job Viewer Tab #1729

Conversation

aestoltm commented May 19, 2023 • edited Loading

Description

Motivation and Context

Tests performed

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanrath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanrath left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aestoltm commented May 19, 2023 •

edited

Loading