Skip to content
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

Display dependency graph using DagreD3 #1817

Merged
merged 12 commits into from
Oct 8, 2018

Conversation

Martchus
Copy link
Contributor

@Martchus Martchus commented Oct 2, 2018

@codecov
Copy link

codecov bot commented Oct 2, 2018

Codecov Report

Merging #1817 into master will decrease coverage by 0.14%.
The diff coverage is 82.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1817      +/-   ##
==========================================
- Coverage   90.36%   90.22%   -0.15%     
==========================================
  Files         139      139              
  Lines        9937     9978      +41     
==========================================
+ Hits         8980     9003      +23     
- Misses        957      975      +18
Impacted Files Coverage Δ
lib/OpenQA/Schema/Result/Jobs.pm 92.54% <100%> (+0.03%) ⬆️
lib/OpenQA/WebAPI.pm 93.93% <100%> (+0.02%) ⬆️
lib/OpenQA/WebAPI/Controller/Test.pm 95.59% <80.55%> (-2.1%) ⬇️
lib/OpenQA/Worker/Commands.pm 85.89% <0%> (-2.57%) ⬇️
lib/OpenQA/Scheduler/Scheduler.pm 86.3% <0%> (-2.29%) ⬇️
lib/OpenQA/WebSockets/Server.pm 90.36% <0%> (-1.84%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0a05f9...693829b. Read the comment docs.

@coolo
Copy link
Contributor

coolo commented Oct 2, 2018

/tests/2068098#settings looks like it can be better :)

@coolo
Copy link
Contributor

coolo commented Oct 3, 2018

I know this WIP, but experimented a little with it:

  • it needs to be in a tab of its own. we have pretty large clusters. Appending this to the huge settings table is no longer fitting. I guess this tab could either disappear or be grayed out for dependencyless jobs
  • the current parents/children can disappear then
  • the NAME is too noisy, especially in large clusters
  • the parallel clusters need to be a little more obvious
  • job status/result needs to be part of the display
  • the dots need to link back to the job

@coolo
Copy link
Contributor

coolo commented Oct 3, 2018

This might be better than what we have now, but I consider it still suboptimal (this manually drafted javascript to experiment with vis.js options

screencapture-file-home-coolo-js-lesmiserables-html-2018-10-03-11_43_59

@coolo
Copy link
Contributor

coolo commented Oct 3, 2018

For the interested: http://stephan.kulow.org/hpc.html

@coolo
Copy link
Contributor

coolo commented Oct 3, 2018

Possibly a network is just the wrong way to think about it. Random links:

https://mermaidjs.github.io/flowchart.html
https://resources.jointjs.com/demos/layout

@coolo
Copy link
Contributor

coolo commented Oct 3, 2018

Playing with mermaidjs:

screencapture-file-home-coolo-js-demo-html-2018-10-03-13_05_49

@Martchus
Copy link
Contributor Author

Martchus commented Oct 3, 2018

Possibly a network is just the wrong way to think about it.

I had the same thought when testing vis.js.

The mermaidjs example looks good. I'll change the PR to use that library. Shouldn't be much work if their API is similar. @coolo Do you have already code or is the example manually drafted?

@coolo
Copy link
Contributor

coolo commented Oct 3, 2018

Their API is completely different (and IMO sucks): http://stephan.kulow.org/mermaid.html

But if we render it like the above, I have the impression we can just as well do it on our own (and better suited to our problem).

So some more links:
https://www.codeproject.com/Articles/16192/Graphic-JavaScript-Tree-with-Layout
https://dagrejs.github.io/project/dagre-d3/latest/demo/clusters.html

@coolo
Copy link
Contributor

coolo commented Oct 3, 2018

DagreD3 is my favorite for now - e.g. it allows us to specify the parallel clusters as tables and then layout it within the graph: https://dagrejs.github.io/project/dagre-d3/latest/demo/dom.html

@Martchus
Copy link
Contributor Author

Martchus commented Oct 4, 2018

@coolo I think I've got the algo to compute the clusters. At least the results look plausible :-)

screenshot_20181004_161458

screenshot_20181004_161531

screenshot_20181004_161515

@coolo
Copy link
Contributor

coolo commented Oct 4, 2018

And 2068098 ? :)

@Martchus
Copy link
Contributor Author

Martchus commented Oct 4, 2018

I only have 2041334 because my database is a little bit older:

screenshot_20181004_165445

BTW: I really need to make the boxes equally sized.

@Martchus Martchus changed the title WIP: Display dependency graph using vis.js WIP: Display dependency graph using DagreD3 Oct 4, 2018
@coolo
Copy link
Contributor

coolo commented Oct 4, 2018

This leaves "the current parents/children can disappear then" - the rest works great.

@Martchus
Copy link
Contributor Author

Martchus commented Oct 4, 2018

And tests are missing or at least need to be adapted.

@Martchus
Copy link
Contributor Author

Martchus commented Oct 5, 2018

The reason why UI test are failing:
screenshot_20181005_131940

@Martchus Martchus changed the title WIP: Display dependency graph using DagreD3 Display dependency graph using DagreD3 Oct 5, 2018
@Martchus
Copy link
Contributor Author

Martchus commented Oct 5, 2018

I added/fixed tests and they pass now. The old table has been removed, too.

Copy link
Contributor

@coolo coolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just nitpicks

@@ -100,6 +100,13 @@
<< https://raw.githubusercontent.com/twbs/bootstrap/v4.1.1/scss/_print.scss
< https://raw.githubusercontent.com/sorich87/bootstrap-tour/6a1028fb562f9aa68c451f0901f8cfeb43cad140/build/css/bootstrap-tour.css

! d3.js
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] It's not just d3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to dagre-d3.js

if (window.location.hash === '#live') {
resumeLiveView();
}
// setup dependency graph if it's the default tab
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these blocks look pretty much repeated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

tooltipText: node.tooltipText,
testResultId: testResultId,
testResultName: testResultName,
fill: "#afa",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little worried to have a colour defined outside of scss. Perhaps this can be inherited from some (otherwise invisible) css?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fill is overridden in CSS anyways. I moved the other fill (which was actually used) to CSS.

@@ -130,6 +130,7 @@ sub startup {
$test_r->get('/')->name('test')->to('test#show');
$test_r->get('/ajax')->name('job_next_previous_ajax')->to('test#job_next_previous_ajax');
$test_r->get('/modules/:moduleid/fails')->name('test_module_fails')->to('test#module_fails');
$test_r->get('dependencies')->name('test_dependencies')->to('test#dependencies');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for consistency either have all $test_r->get calls with / or none.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

* Remove comment in SCSS which would be rendered into CSS
* Don't include fonts twice
* Fix duplicated code
* Define all colors in CSS
* Rename asset (d3.js => dagre-d3.js)
@coolo coolo merged commit f526e2b into os-autoinst:master Oct 8, 2018
@Martchus Martchus deleted the dependency_graph branch October 8, 2018 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants