Page MenuHomePhabricator

[LNT] Refactored the Graph page to use the library plotly instead of jQuery.flot
ClosedPublic

Authored by kpdev42 on Oct 7 2021, 2:10 AM.

Details

Summary

Depends on D114332, D114333

Added the library Plotly.
Updated the page Graph to use plotly instead of jQuery.flot.
Note the tooltip window may be closed by left doubleclick on the graph area.
Implemented multiple Y axis, Dates as X axis, cumulative minimum, etc.

OS Laboratory. Huawei Russian Research Institute. Saint-Petersburg

Diff Detail

Repository
rLNT LNT

Event Timeline

kpdev42 created this revision.Oct 7 2021, 2:10 AM
kpdev42 requested review of this revision.Oct 7 2021, 2:10 AM

Ping

Anyway this could be split? This is a giant patch. Note: I am not guaranteeing any review after splitting because I am not familiar with that part of LNT. Happy to give it a try if the patch can be split into small enough patches.

asl added a subscriber: asl.Oct 25 2021, 10:26 AM

@thopre I doubt the refactorings of this kind (change one plotting framework into another) could be split into smaller patches just by the nature of the change, you cannot have "half plot" and one essentially needs all the features ready for the review. So, such large patches are inevitable

@thopre I doubt the refactorings of this kind (change one plotting framework into another) could be split into smaller patches just by the nature of the change, you cannot have "half plot" and one essentially needs all the features ready for the review. So, such large patches are inevitable

I was afraid that'd be the case indeed. I'll try to give it a look but please bear with me as I'm not sure when I'll be able to set aside enough time to review this.

As a start, split the non-patch related changes out. Specifically, at least split the changes in: test_matrix_page.py, test_api.py, the renames, the error message updates in views.py, the refactoring in api.py.

We need test coverage in all this code being added as well.

Move some of this out into a series of refactorings, before the main final change, that will help make it simpler to reason about the changes.

I have gone through these code areas a lot in the past, I am happy to review this.

As a start, split the non-patch related changes out. Specifically, at least split the changes in: test_matrix_page.py, test_api.py, the renames, the error message updates in views.py, the refactoring in api.py.

Note Plotly supports datetime and series for X-axis. Both types are implemented here. Refactoring in api.py and such is necessary to process the data correctly. Note some graph data is prepared during the page rendering and the rest graph data is requested by JS from the browser via api. So api.py must be updated. test_api.py depends on api.py

Old behavior: revisions 1.0, 2.3 and 45.67 were placed on X-axis disproportionately. 1.0 and 2.3 stick together, but the distance between 2.3 and 45.67 is too long.

New behavior: everything is located on the X-axis in proportion as series. Revisions may not contain digits at all. And https://reviews.llvm.org/D109577 looks reasonable after this patch.

We need test coverage in all this code being added as well.

What code being added do you mean?
lnt_regression.js is just a copy of the old version of lnt_graph.js
The code for jQuery.Flot has been replaced with the code for Plotly.
All tests are the same, but some tests were updated.
Some code from Graph and Matrix has been combined in few helper functions like load_plot_parameter(). Renaming graph_parameters and data_parameters to plot_parameters is a part of it.

Right, it is possible to separate error messages and do not touch test_matrix_page.py for now (test_matrix_page.py depends on error messages.). But the half of error messages is in the new code. I don't see a reason for moving the half of error messages to a separate patch.

Move some of this out into a series of refactorings, before the main final change, that will help make it simpler to reason about the changes.

The final goal is the quality and stable code, right? It seems the new implementation is already tested and worked well. It is very easy to break something trying to split it into a series of refactorings.

As a start, split the non-patch related changes out. Specifically, at least split the changes in: test_matrix_page.py, test_api.py, the renames, the error message updates in views.py, the refactoring in api.py.

We need test coverage in all this code being added as well.

Move some of this out into a series of refactorings, before the main final change, that will help make it simpler to reason about the changes.

Thanks for suggestions. I will try to split this patch

kpdev42 updated this revision to Diff 383005.Oct 28 2021, 5:57 AM
kpdev42 edited the summary of this revision. (Show Details)

Updated patch after landing https://reviews.llvm.org/D112607 and https://reviews.llvm.org/D112525

P.S.: dear reviewers, please do not commit this patch without mentioning me as an author, because it may raise some legal questions from my company )

Updated patch after landing https://reviews.llvm.org/D112607 and https://reviews.llvm.org/D112525

P.S.: dear reviewers, please do not commit this patch without mentioning me as an author, because it may raise some legal questions from my company )

Oh my bad, normally arc land keeps the author information. I did an amend to include the formatting changes but that should also keep the author information, not sure what happened with https://reviews.llvm.org/D112607

This still needs to be more targeted.

Can you make the refactoring of lnt_graph.js and lnt_regression.js as a separate change. As well as the refactoring in lnt/server/ui/api.py, lnt/server/ui/templates/v4_new_regressions.html.

I just made a few comments while skimming this.

lnt/server/ui/views.py
884

Line below needs indenting matched. Did this pass flake8?

1127

Delete comment.

lnt/server/ui/views_util.py
15 ↗(On Diff #383005)

Why are you renaming StringIO to CsvStringIO, then writing json to it? Maybe keep this as StringIO.

20 ↗(On Diff #383005)

Need some docs. Why is this needed? Why not io.TextIOWrapper?

36 ↗(On Diff #383005)

Doc comment here.

36 ↗(On Diff #383005)

everything is json in a web app. Better name?

61 ↗(On Diff #383005)

What is this second code path for? these both return json?

71 ↗(On Diff #383005)

Docs.

71 ↗(On Diff #383005)

I'm confused by the return type of this.

71 ↗(On Diff #383005)

This needs some unit tests.

kpdev42 updated this revision to Diff 388753.Nov 21 2021, 5:23 AM

Depends on https://reviews.llvm.org/D114332, https://reviews.llvm.org/D114333

Some changes were moved to https://reviews.llvm.org/D114332, https://reviews.llvm.org/D114333
Note views.py passes flake8.
CSV export was removed to reduce changes.
Note JSON may be returned as a regular HTTP response or the file downloading may be triggered.

Applying this locally, the regression triangles seem to be gone, was this intentionally?

Applying this locally, the regression triangles seem to be gone, was this intentionally?

Please note this patch is now separated to few smaller patches as asked by other reviewers. Now it depends on D114332 and D114333. Regression triangles are requested from JS via graph API. The graph API is updated in D114333. Please apply both D114332 and D114333 before this patch to keep all functionality. Probably some rebase is necessary.

kpdev42 edited the summary of this revision. (Show Details)Nov 30 2021, 9:50 PM

@kpdev42 it looks like @cmatthews left some comments for you that haven't been addressed yet. Would be good to address them so we can move this forward a bit.

Can you update this patch now that some of the changes have landed?

slydiman added a comment.EditedDec 3 2021, 11:29 AM

Can you update this patch now that some of the changes have landed?

It seems everything is updated 2 weeks ago. Probably something is cached in your browser. Try Ctrl+F5.
This patch changes only 3 files: lnt_graph.js, v4_graph.html and views.py.
The new file views_util.py with CSV writer is removed to minimize changes. So all comments regarding views_util.py are not actual now.

Note views.py passes flake8.

Oh yes! I did have to load the page, then do a refresh before the old parts of the patch went away.

Wow, v4_graph is complex!

Thank you for breaking this up! Much easier to review. This looks good to me now.

lnt/server/ui/views.py
869–876

For functions that are being significantly changed, can you add type annotations.

We are trying to slowing introduce types into the system. I realize the original functions did not have them, but now is the time to add them.

That could be in a later commit though, if that is easier.

1083

maybe metric_names? Metric is a very overloaded term in this system!

cmatthews accepted this revision.Dec 3 2021, 1:01 PM
This revision is now accepted and ready to land.Dec 3 2021, 1:01 PM