You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by PBGraff <gi...@git.apache.org> on 2018/01/14 02:45:04 UTC
[GitHub] tinkerpop pull request #777: TINKERPOP-1862 TinkerMessenger proper handling ...
GitHub user PBGraff opened a pull request:
https://github.com/apache/tinkerpop/pull/777
TINKERPOP-1862 TinkerMessenger proper handling of Direction.BOTH
New PR targeted to `tp32` branch. Fixes `TinkerMessenger` handling of direction to get the correct messages. Also now adds a set of tests to `GraphComputerTest` to test this message passing in a simple graph with a simple vertex program in all directions (`IN`, `OUT`, and `BOTH`).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/PBGraff/tinkerpop TINKERPOP-1862
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/777.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #777
----
commit 5e157faebfe971c3ef06ac5365993052021a95fb
Author: Graff, Philip B <ph...@...>
Date: 2018-01-04T18:27:44Z
TinkerMessenger proper handling of Direction.BOTH
commit 01a9ee5ae80fd429726d0722edae3195d9a625c5
Author: Graff, Philip B <ph...@...>
Date: 2018-01-14T02:32:29Z
Adding simple graph computer test of proper message passing in all 3 directions
----
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/777
When I run the integration tests with `docker/build.sh -t -i` I get some failures in spark - do they fail for you too @PBGraff ?
```text
[ERROR] Tests run: 619, Failures: 0, Errors: 3, Skipped: 149, Time elapsed: 237.084 s <<< FAILURE! - in org.apache.tinkerpop.gremlin.spark.process.computer.SparkGraphComputerProcessIntegrateTest
[ERROR] testMessagePassingOut(org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest) Time elapsed: 0.001 s <<< ERROR!
java.lang.UnsupportedOperationException: Graph does not support adding vertices
at org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest.runTest(GraphComputerTest.java:2666)
at org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest.testMessagePassingOut(GraphComputerTest.java:2637)
[ERROR] testMessagePassingBoth(org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest) Time elapsed: 0 s <<< ERROR!
java.lang.UnsupportedOperationException: Graph does not support adding vertices
at org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest.runTest(GraphComputerTest.java:2666)
at org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest.testMessagePassingBoth(GraphComputerTest.java:2652)
[ERROR] testMessagePassingIn(org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest) Time elapsed: 0.001 s <<< ERROR!
java.lang.UnsupportedOperationException: Graph does not support adding vertices
at org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest.runTest(GraphComputerTest.java:2666)
at org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest.testMessagePassingIn(GraphComputerTest.java:2622)
[INFO]
[INFO] Results:
[INFO]
[ERROR] Errors:
[ERROR] org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest.testMessagePassingBoth(org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest)
[ERROR] Run 1: GraphComputerTest.testMessagePassingBoth:2652->runTest:2666 ? UnsupportedOperation
[ERROR] Run 2: GraphComputerTest.testMessagePassingBoth:2652->runTest:2666 ? UnsupportedOperation
[INFO]
[ERROR] org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest.testMessagePassingIn(org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest)
[ERROR] Run 1: GraphComputerTest.testMessagePassingIn:2622->runTest:2666 ? UnsupportedOperation
[ERROR] Run 2: GraphComputerTest.testMessagePassingIn:2622->runTest:2666 ? UnsupportedOperation
[INFO]
[ERROR] org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest.testMessagePassingOut(org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest)
[ERROR] Run 1: GraphComputerTest.testMessagePassingOut:2637->runTest:2666 ? UnsupportedOperation
[ERROR] Run 2: GraphComputerTest.testMessagePassingOut:2637->runTest:2666 ? UnsupportedOperation
[INFO]
[INFO]
[ERROR] Tests run: 1625, Failures: 0, Errors: 3, Skipped: 267
```
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/777
#794 and #795 have both merged now. @PBGraff you now have a test graph with a single simple self-loop subgraph that you can use (and obviously add other test subgraphs as needed). hope that helps move this PR forward. please let me know if you have any questions
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Posted by PBGraff <gi...@git.apache.org>.
Github user PBGraff commented on the issue:
https://github.com/apache/tinkerpop/pull/777
I don't have an environment for testing with Spark right now, so I was unable to run those tests. They're failing because for the test I added I want to create my own graph and add two vertices and two edges to it (see `runTest(Direction)` at line 2665 of `GraphComputerTest.java`). Is there a better way to do this that will be acceptable for Spark where it seems the graph is immutable? I was trying to not have to create another GraphML/GraphSON/Gryo graph to be stored.
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/777
The tests don't require spark. You just need to run the integration tests. with docker it's as easy as: `docker/build.sh -t -i`. in a local maven environment, you can `mvn verify -pl spark-gremlin -DskipIntegrationTests=false` after a local build. I don't know spark-gremlin too well, so i may have to dig into the testing stuff a bit to give you an answer on the "graph test data" issue. i'll don't think i can get to that today....not sure if anyone else can chime in with an answer.
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/777
Unfortunately, we don't have any self-loops in our sample graphs. @spmallette That's really something we should consider to add.
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/777
dar - a new graph - serious yak shaving going on with pull requests lately.
perhaps we create a "TEST" graph that can contain many disconnected subgraphs that we can add to and adjust as needed. When we used it in our tests we would just always take care to blind any test traversals to the parts of the graph that didn't matter. That way, we could have just abstract stuff in there like, a self-loop, a tree, etc. Nothing would have to have meaning like "The Crew" or "Modern". does that make sense?
---
[GitHub] tinkerpop pull request #777: TINKERPOP-1862 TinkerMessenger proper handling ...
Posted by PBGraff <gi...@git.apache.org>.
Github user PBGraff closed the pull request at:
https://github.com/apache/tinkerpop/pull/777
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/777
I agree this test should run for all GraphComputers, but the test may not try to mutate the graph, as the test suite will run the test against a whole bunch of immutable graphs.
Assuming that all we need is a pair 2 vertices connected via 2 edges in opposite directions, we could use the Grateful Dead graph; this pattern occurs 1476 times in it.
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Posted by PBGraff <gi...@git.apache.org>.
Github user PBGraff commented on the issue:
https://github.com/apache/tinkerpop/pull/777
I think it's best to include this test for all `GraphComputer` implementations to ensure that they are doing message passing correctly. The problem was found with `TinkerMessenger`, but the test should be there for code coverage.
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/777
I'm posting this to all open PRs of current relevance - this PR needs to be rebased against the branch it is targeted against given a broken python dependency that was published to pypi a day or so ago. I've pushed a fix on tp32 and master at this point and David Brown is working on getting an issue raised with the project that initiated the problem.
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Posted by PBGraff <gi...@git.apache.org>.
Github user PBGraff commented on the issue:
https://github.com/apache/tinkerpop/pull/777
Thanks, @spmallette. Regarding the re-basing, I think I just managed that...
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/777
Absolutely, I like the idea.
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Posted by PBGraff <gi...@git.apache.org>.
Github user PBGraff commented on the issue:
https://github.com/apache/tinkerpop/pull/777
Great, thanks. I haven't been able to work on this recently but having that merged will help. I'll check it out and update my test as well as fix the git history of the branch for this and #772.
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Posted by PBGraff <gi...@git.apache.org>.
Github user PBGraff commented on the issue:
https://github.com/apache/tinkerpop/pull/777
Tests pass now, good to go from me.
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/777
I guess I'll create a new issue for this in JIRA. crazy
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/777
I looked into this one some more. This was a change to the `TinkerMessenger` so technically I would think that this is a change specific to TinkerGraph, in which case the test could be aligned solely to TinkerGraph. But....it makes me wonder if other `GraphComputer` (like Spark's) work correctly. I'm hesitant to just bind this test to TinkerGraph if it's good to test to have elsewhere.
@dkuppitz any thoughts on this?
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Posted by PBGraff <gi...@git.apache.org>.
Github user PBGraff commented on the issue:
https://github.com/apache/tinkerpop/pull/777
I'll look, but I was also particularly interested in having a self-loop edge to make sure that those are handled correctly as well.
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/777
I just added the new toy graph infrastructure on #794 - as soon as that merges then we can finish this one up.
@PBGraff something still sorta looks fishy here with the commit history. my expectation would be that this PR would have no commits except your own, but i still see some of mine (and others) in there. I don't know the full state of your fork, so it's hard to give advice, but perhaps you need to:
1. update your fork's tp32 branch with the latest from tinkerpop
2. `git checkout tp32`
3. `git checkout -b TINKERPOP-1862-revised`
4. cherry pick just your commits from TINKERPOP-1862 over to TINKERPOP-1862-revised
5. kill the old TINKERPOP-1862 (local and remote) and rename it TINKERPOP-1862-revised appropriately by dropping the "-revised" then push that
Then going forward all you should have to do is keep your fork's tp32 branch updated and periodically: `git rebase origin/tp32` on the TINKERPOP-1862 branch which will continually take all your changes and replay them on the head of your fork's tp32.
If you have some other more elegant git solution, that's cool too....just providing the hammer solution to the problem in case you can't figure out anything better.
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Posted by PBGraff <gi...@git.apache.org>.
Github user PBGraff commented on the issue:
https://github.com/apache/tinkerpop/pull/777
@spmallette Updated PR from https://github.com/apache/tinkerpop/pull/770
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/777
> we could use the Grateful Dead graph; this pattern occurs 1476 times in it.
perfect - i was wondering if Grateful Dead would work earlier. @PBGraff can you perhaps find a pattern in that graph that will allow you to just use that existing data set?
---