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

New theme for luigi visualiser #1086

Merged
merged 32 commits into from
Sep 17, 2015
Merged

New theme for luigi visualiser #1086

merged 32 commits into from
Sep 17, 2015

Conversation

stephenpascoe
Copy link

This is a new theme for the visualiser accessible through .../index.lte.html. The theme takes the d3 theme and adds jQuery DataTables for displaying a filterable table of tasks and AdminLTE for styling.

Key features are:

  • Table paging
  • Search filter by any text in the table
  • Info boxes displaying count of task status (done, running, etc.) are selectable to filter the table
  • Sidebar displaying task names with counts are selectable to filter the table
  • Exception reports are accessible via expandable rows

I can imagine improving the layout of the "Parameters" column and making more use of the expandable rows but we are already finding this very useful in production.

With 3 separate versions of index.html and several new dependencies I think it would be worth refactoring the /static structure and finding a neater way of selecting themes. If you agree I'll take a look at it.

@Tarrasch
Copy link
Contributor

Pics or it didn't happen :p

@stephenpascoe
Copy link
Author

Screenshot with one task name selected.

luigi_lte_theme


<meta name="viewport" content="width=device-width, initial-scale=1.0">

<style type="text/css">
Copy link
Contributor

Choose a reason for hiding this comment

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

can these styles be static files?

Copy link
Author

Choose a reason for hiding this comment

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

I just copied the inline style from index.d3.html. I can definitely factor them into a separate file. It would be good to do a more general refactor of the 3 themes at the same time as they are rather mixed up at the moment.

@Tarrasch
Copy link
Contributor

Does this patch come with some documentation of how to set it up and stuff? Or are you just supposed to browse to static/visualizer/index.lte.html?

@erikbern
Copy link
Contributor

This is beautiful

@stephenpascoe
Copy link
Author

@erikbern thanks!
@Tarrasch I'm working on configuration now to switch themes. No setup should be needed, just navigate to index.lte.html

@stephenpascoe
Copy link
Author

I have now made a more general refactoring of the visualiser files to put each theme in its own directory. The theme can be set with the "visualizer_theme" option as either "d3", "svg" or "lte".

I've kept the "visualization_graph" option for backward compatibility but it is a bit confusing having both of these options. I'm wondering whether it is acceptable to have one option named "visualizer_theme" instead, particularly as this option is not in the docs yet. Once I get feedback on that I can write a section in the configuration docs.

@erikbern
Copy link
Contributor

wait we have three themes now? can we deprecate one or two of them and change the default?

@stephenpascoe
Copy link
Author

Sorry, I'm conflating 2 things. I've written the new theme to use the d3 visualisation. My understanding is that some people want to continue to use the svg visualisation, therefore I've merged the concepts into 3 themes:

  1. "svg": traditional style with svg visualisation
  2. "d3": traditional style with d3 visualisation
  3. "lte": new style with d3 visualisation

I think it's too much work to make the visualisation independent from the style and I'm using the d3 visualisation in my environment. So, it's a question of what people want. We could remove no. 2?

@erikbern
Copy link
Contributor

I think we should remove #1 and #2, your visualizer looks much better! I'm sure people will disagree though

@stephenpascoe
Copy link
Author

OK, I'm happy with that. My only hesitation is that I haven't tried the d3 visualisation with very large dependency trees.

I'll make mine the default and retain the original as an option.

@gpoulin
Copy link
Contributor

gpoulin commented Jul 24, 2015

I agree with @erikbern on this one. The frontend is just a monitoring tool. I don't having many themes bring so much, but it complexifies any change to the frontend.

@interskh
Copy link
Contributor

We tried D3 one a while back. It doesn't work well with large dependency trees unfortunately :(

But I like this new theme!

@Tarrasch
Copy link
Contributor

@interskh oh dang, I hoped D3 would be more efficient :-/

@stephenpascoe
Copy link
Author

I've had a look at scalability of the D3 visualisation.

  • There is a bug in zoom/pan which resets the view periodically. This makes navigating trees a pain if it doesn't fit in the window.
  • Up to a few 100 leaf nodes D3 is passable. Draw speed is ok. All nodes at a given depth are rendered at the same x-coord which makes wide trees tall and thin.
  • At ~1000 leaf nodes redraw speed is too slow.

It would be useful to know what constitutes a large tree for others.

This PR could be changed to use the old visualisation but supporting both would require refactoring and add complexity. Right now, I'm finding the D3 vis useful so I'd like to find a way to make it work. I'll look more closely at the code to consider options.

I'm going to be offline for most of the next 2 weeks so won't get back to this for a while.

@interskh
Copy link
Contributor

For an example, We have a task depends on a month of hourly task, each of which depends on 2 more tasks. So there are 2k nodes and D3 just refuses to draw anything (or just hang), svg actually renders pretty fast (despite not pretty, but it is still useful) :(

Another thing is that the check for "Show Upstream Dependencies" doesn't seem to work on d3.

@tym-xqo
Copy link
Contributor

tym-xqo commented Jul 30, 2015

I can write this as a separate issue elsewhere if that would be better, but as long as we're griping about d3: I've noticed all the above issues with zoom and large graphs, and plus the "Show Error Trace" button seems to show just an empty lightbox—at least when I use it.

@kwent
Copy link

kwent commented Aug 6, 2015

This is beautiful ! Goob job can't wait to see that merge with themes supports !

@erikbern
Copy link
Contributor

erikbern commented Aug 6, 2015

merge?

@Tarrasch
Copy link
Contributor

Tarrasch commented Aug 6, 2015

I think we're waiting for @stephenpascoe to come back from vacation and reorganize this PR a bit. I'm also not sure who've actually closely reviewed this.

@stephenpascoe
Copy link
Author

Just to let you know I'm back and have some time to work on this PR this week. I've sped D3 up a little and have found a way to get both visualisations switchable in the same page. I hope to update the PR this week.

@interskh
Copy link
Contributor

👍

@Tarrasch
Copy link
Contributor

👍 indeed :)

@beeva-luisgonzalez
Copy link

looking good!

@stephenpascoe stephenpascoe force-pushed the master branch 3 times, most recently from 621e1f6 to 077bee8 Compare August 20, 2015 14:27
@stephenpascoe
Copy link
Author

I have removed the theme logic so there is only one theme (the new one). All references to "themes" have been removed.

  • You can switch between SVG and D3 visualisations using a switch on the page
  • I changed the D3 layout algorithm to one which draws a little quicker. ~500 nodes are rendered in ~4s. The layout is slightly less optimal but looks OK to me.
  • I removed the automatic refresh code. This caused problems when dragging the graph and was infeasible for medium-sized graphs
  • I fixed display of the inverse dependency graphs with D3

@Tarrasch
Copy link
Contributor

Cool! Awesome with a switch on the page.

I removed the automatic refresh code. This caused problems when dragging the graph and was infeasible for medium-sized graphs

Great that you prioritize stability. :)

Stephen Pascoe added 14 commits September 15, 2015 17:55
Towards resolving #2

The API is queried on a per-category basis (PENDING, UPSTREAM_FAILED, etc.).  Therefore we can calculate
the real number of tasks for each category but cannot keep the per-family counts accurate if max-shown-tasks
is exceeded.  The warning tries to make this clear.
remember visType across reloads.
The behaviour is different on Firefox to Chrome/Safari.  For some reason firefox remembers the state of the checkbox on reload but the others reset it.  Either way, this update keeps the UI consistent.
@stephenpascoe
Copy link
Author

I've rebased and added 1 commit which fixes the newline issue. It tweaks commit f5267517.

Re @daveFNbuck's other points. Once this is merged I'd be keen to refactor the javascript. It is an amalgamation of the original, the D3 code and my changes; there is plenty of dead code; the callbacks are messy. These other features would be easier to do after that. I will need to focus on other things for a few weeks though.

@daveFNbuck
Copy link
Contributor

Looks good to me if you just change line 221 in visualiserApp.js to

worker.tasks = worker.running.map(taskToDisplayTask);

and fix whatever caused travis to break

@Tarrasch
Copy link
Contributor

Cool, this is soon reaching completion it seems. :)

@Tarrasch
Copy link
Contributor

Cool. Is there anything left to be done for this @stephenpascoe?

@stephenpascoe
Copy link
Author

IMO It's done.

Tarrasch added a commit that referenced this pull request Sep 17, 2015
New theme for luigi visualiser
@Tarrasch Tarrasch merged commit 9889f11 into spotify:master Sep 17, 2015
@Tarrasch
Copy link
Contributor

Huzzah!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

Awesome work @stephenpascoe!!

@ulzha
Copy link
Contributor

ulzha commented Sep 17, 2015

Well done! Super clear legend and all.

@erikbern
Copy link
Contributor

champagne

@stephenpascoe
Copy link
Author

Thanks all, and thanks to @beeva-luisgonzalez for the D3 code.

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.