Skip to content

Conversation

@conker84
Copy link
Contributor

@conker84 conker84 commented Mar 12, 2017

What is this PR for?

This issue is about a new network visualization that can leverage the Property Graph Model (https://github.com/tinkerpop/gremlin/wiki/Defining-a-Property-Graph), but also simple graphs in order to provide a set of base apis that can be used by graph dbs (like Neo4j) or graph processing frameworks (like GraphX or Giraph).

What type of PR is it?

[Feature]
Is related to the #1582

Todos

  • - Added the intepreter apis to manage graphs (under the pakage org.apache.zeppelin.interpreter.graph)
  • - Added the frontend apis to manage graphs (via d3js)

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-2222

How should this be tested?

You can download this notebook to test the PR

Screenshots (if appropriate)

zeppelin-pr-screen

Video

zeppelin

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? Yes

@1ambda
Copy link
Member

1ambda commented Mar 13, 2017

@conker84 Pretty graph! I have one question.

  • It looks like new display system that works with backend interpreter.
  • and it's enabled when we click the network visualization icon.

So, what happens when we click 'table` or other visualization icons in the situation? (the screenshot you attached.)

@conker84
Copy link
Contributor Author

@1ambda the graph is flatten by javascript apis so we can leverage the other visualizations provided by Zeppelin
I'll add a video asap to show how it works!

@tae-jun
Copy link
Contributor

tae-jun commented Mar 13, 2017

@conker84 FYI, you can simply just take a GIF using software called LICEcap, and upload it directly here with drag and drop :)

* each edge has a label that denotes the type of relationship between its two vertices.
* each edge has a collection of properties defined by a map from key to value.

<img src="https://github.com/tinkerpop/gremlin/raw/master/doc/images/graph-example-1.jpg" />
Copy link
Member

Choose a reason for hiding this comment

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

sidenote: would be great to support tinkerpop? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any special reason to use github hosted image instead of adding a new one to Zeppelin source?

Copy link
Contributor Author

@conker84 conker84 May 9, 2017

Choose a reason for hiding this comment

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

@AhyoungRyu sorry i had lost your comment, no there is no special reason (it's taken from the Property Graph Model page), if you think is better I can import the image inside the project

* "data": the data attached to the edge;
* "labels": a map (K, V) where K is the node label and V is the color of the node;
* "directed": (true/false, default false) wich tells if is directed graph or not;
* "types": a *distinct* list of the edge types of the graph
Copy link
Member

Choose a reason for hiding this comment

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

could you include a sample of the json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

<h4>Globals</h4>
<div class="form-check col-xs-4">
<div class="form-group">
<label for="{{$id}}_charge">Minumin scale to show node and edge labels</label>
Copy link
Member

Choose a reason for hiding this comment

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

Minumin -> Minimum?

@1ambda
Copy link
Member

1ambda commented Mar 15, 2017

I will test and give you feedback soon.

@conker84
Copy link
Contributor Author

Hi guys any news/feedback?

@conker84
Copy link
Contributor Author

conker84 commented Apr 3, 2017

Hi guys i'm still here, let me know if i can do something to simplify the review!

@jongyoul
Copy link
Member

jongyoul commented Apr 3, 2017

@conker84 Thanks for this nice contribution. We introduced Spell in the latest release. Spell helps you contribute this kind of wonderful visualization as easier than now. @1ambda Can you guide him to convert it to Spell?

@conker84
Copy link
Contributor Author

conker84 commented Apr 3, 2017

@jongyoul thanks for the answer, i checked the Spel and the Visualization and i understood they are front-end only. This contribution is a mixed front/back-end becouse it want to provide a set of base api in order to enable graph-databases's exploration (like Neo4j or others) with Zeppelin.
This contribution is the first step of #1582 the second is to implement the Neo4j Interpreter on top to the GraphResult apis.
If you guys think there is a more valuable solution to this i'm happy to talk about it!

@felixcheung
Copy link
Member

I think this is introducing a new result type from interpreter on the backend side that could be useful for this new Neo4J interpreter and other graph-base ones in the future?

@conker84
Copy link
Contributor Author

conker84 commented Apr 4, 2017

Yes it is!

@1ambda
Copy link
Member

1ambda commented Apr 4, 2017

Really sorry for late reply. I missed notification for a while.

  • Spell can use JSON output from neo4j interpreter with sigma.js, vis.js
  • Having default network display system using generalized graph model (Property Graph Model) would be good idea.
  • Personally, hope to get more refined, pretty graph UI instead of d3 network.

@conker84
Copy link
Contributor Author

conker84 commented Apr 4, 2017

The #1582 initally was with sigma.js but i found a strange memory consuption that broke the browser after the #1529 so i migrated to d3 network visualization, i chose d3 instead of vis.js in order to reduce project's dependencies.
If you want i can try with vis.js!

@1ambda
Copy link
Member

1ambda commented Apr 5, 2017

It's just my opinion. Thus, it's ok to use d3

  • if you think it's reasonable 👍
  • and the most important thing of this PR is not UI

@conker84
Copy link
Contributor Author

conker84 commented Apr 6, 2017

We are talk about graphdb, the UI is an important part of the PR, as well as the new interpreter apis!
I'm looking forward to your feedback.

@1ambda
Copy link
Member

1ambda commented Apr 7, 2017

UI can be improved in the following PRs. So it's ok not to resolve the whole things in this PR.

@conker84
Copy link
Contributor Author

conker84 commented Apr 7, 2017

Let me know if you want go ahead with this PR or i have to close it!

@conker84
Copy link
Contributor Author

conker84 commented Apr 7, 2017

Ok! So i'll rebase the code ASAP.

@1ambda
Copy link
Member

1ambda commented Apr 7, 2017

FYI, i didn't mean you have to create other PRs. Others can improve UI.

@conker84
Copy link
Contributor Author

Done!

@1ambda
Copy link
Member

1ambda commented Apr 14, 2017

@conker84 you should rebase based on master, not other branch. Currently, your change mixed with others.

image

@conker84
Copy link
Contributor Author

Is it now ok?

@conker84
Copy link
Contributor Author

I think i made some error with git commits please can someone help me?

@conker84
Copy link
Contributor Author

Yes i made the rebase but it starts from the history dc7b100 instead of 43352ae and i can't understand why.
Have you some ideas?
Thanks

@zjffdu
Copy link
Contributor

zjffdu commented Apr 27, 2017

Please rebase it upon apache master branch

@conker84
Copy link
Contributor Author

conker84 commented Apr 30, 2017

Is it now ok? (I don't think so)
I'm rebasing from the master branch i can't figure out where is the problem :(

@1ambda
Copy link
Member

1ambda commented May 4, 2017

Hi, @conker84

Here is failed JOB and looks irrelevant. But could you make CI green?

you can click restart button in travis.

@conker84
Copy link
Contributor Author

conker84 commented May 4, 2017

i tried but it still fails...

@1ambda
Copy link
Member

1ambda commented May 4, 2017

  1. Regarding to continuous-integration/appveyor/pr — AppVeyor build failed, you should rebase. Since master has .appvepor.yml file.

  2. In case of Jenkins, you are failing in the profile 3 (selenium test)

Tests in error: 
  AuthenticationIT.testGroupPermission:190->AbstractZeppelinIT.pollingWait:94 » Timeout
  InterpreterIT.testShowDescriptionOnInterpreterCreate:69 » ElementNotVisible El...

They might be due to flaky tests. You can simply pass them by clicking restart button in travis.

@conker84
Copy link
Contributor Author

conker84 commented May 5, 2017

It should be ok now

@conker84
Copy link
Contributor Author

Hi, could you please update me about the status of the review? Thanks!

@conker84
Copy link
Contributor Author

No news?

@jongyoul
Copy link
Member

@conker84 Hi, I have a question. I'm not sure but classes under org.apache.zeppelin.interpreter.graph is used only for testing it. Can you verify it? If it does, can you move these classes under test?

@conker84
Copy link
Contributor Author

@jongyoul this contribution is the result of the split of #1582 in two little PR; the classes under org.apache.zeppelin.interpreter.graph provide the basic abstraction that can be used by graph dbs (like Neo4j, OrientDB, ArangoDB etc...) or graph processing frameworks (like GraphX or Giraph) and they will be used by the second part of the contribution which is the Neo4j interpreter, if you think is more appropriate I can move these classes into the second contribution.

@conker84
Copy link
Contributor Author

Still no news?

@conker84
Copy link
Contributor Author

conker84 commented Jun 6, 2017

Hi guys,
i'm still waiting an answer, maybe there is no interest to this PR, it's ok for me but just give me some feedback. Sorry if i bother you but I need this with the Neo4jIntepreter for a small PoC that i have to develop so I need infos in order to plan my work.
Thanks

@jongyoul
Copy link
Member

jongyoul commented Jun 7, 2017

Oh, I'm so sorry for the late reply. I've missed it. I think you'd better to move those classes into another PR

@jongyoul
Copy link
Member

jongyoul commented Jun 8, 2017

Thanks, Will merge it after CI become green

@jongyoul
Copy link
Member

jongyoul commented Jun 9, 2017

Can you make CI green?

@felixcheung
Copy link
Member

looks like everything is green on travis but jenkins isn't picking them up
https://travis-ci.org/conker84/zeppelin/builds/240773799

@jongyoul
Copy link
Member

jongyoul commented Jun 9, 2017

@felixcheung Yes, I already checked it but I was confused the branch name is master. :-) Thanks

@asfgit asfgit closed this in 4392648 Jun 9, 2017
@jongyoul
Copy link
Member

jongyoul commented Jun 9, 2017

@conker84 I found it's your first contribution. Thank you.

@felixcheung
Copy link
Member

felixcheung commented Jun 9, 2017 via email

@conker84
Copy link
Contributor Author

Thank you guys now I'll proceed with the second part of the original PR. The Neo4j's interpreter:)

@1ambda
Copy link
Member

1ambda commented Jun 19, 2017

@conker84 Hi, could you take care this issue might be related without NETWORK viz?

  <div ng-if="type == 'TABLE' || type == 'NETWORK'" class="btn-group">
    <button type="button" class="btn btn-default btn-sm"
            ng-repeat="viz in builtInTableDataVisualizationList track by $index"
            ng-if="viz.supports.indexOf(type) > -1"
            ng-class="{'active' : viz.id == graphMode && !config.helium.activeApp}"
            ng-click="switchViz(viz.id)"
            tooltip-placement="bottom" uib-tooltip="{{viz.name ? viz.name : ''}}"
            ng-bind-html="viz.icon">
    </button>
  </div>

@conker84
Copy link
Contributor Author

Hi @1ambda I created the #2421

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.

7 participants