[GitHub] tinkerpop issue #772: TINKERPOP-1861 Modify VertexProgram Builder to take va...
Github user PBGraff commented on the issue: https://github.com/apache/tinkerpop/pull/772 No worries, thanks! ---
[GitHub] tinkerpop issue #772: TINKERPOP-1861 Modify VertexProgram Builder to take va...
Github user PBGraff commented on the issue: https://github.com/apache/tinkerpop/pull/772 @spmallette I'm not sure I'll come back to this exactly. I was thinking about it recently, actually, and I think just providing a `create()` which calls `create(null)` in the VertexProgramBuilder would be useful for Java users. This is a simpler change than making it varagrs and less likely to cause breaking changes and probably covers 99+% of use cases. ---
[GitHub] tinkerpop issue #829: TINKERPOP-1888 Extend max and min to all Comparable pr...
Github user PBGraff commented on the issue: https://github.com/apache/tinkerpop/pull/829 Thank you! ---
[GitHub] tinkerpop issue #772: TINKERPOP-1861 Modify VertexProgram Builder to take va...
Github user PBGraff commented on the issue: https://github.com/apache/tinkerpop/pull/772 @spmallette Thanks for the heads-up, but I don't think I'll have a chance to get to it before then. It isn't so urgent it can't wait for the next release as it's code style as opposed to a bug. ---
[GitHub] tinkerpop issue #804: TINKERPOP-1862 Messenger proper handling of Direction....
Github user PBGraff commented on the issue: https://github.com/apache/tinkerpop/pull/804 This looks good to me. Thanks for fixing the Spark and Giraph versions as well, @spmallette. ---
[GitHub] tinkerpop issue #801: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Github user PBGraff commented on the issue: https://github.com/apache/tinkerpop/pull/801 OK, I've made that update and now this build passes. ---
[GitHub] tinkerpop issue #801: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Github user PBGraff commented on the issue: https://github.com/apache/tinkerpop/pull/801 I'm not sure why this is failing, it's due to something with the javascript. I don't know how my changes potentially affected that. ---
[GitHub] tinkerpop issue #801: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Github user PBGraff commented on the issue: https://github.com/apache/tinkerpop/pull/801 Done ---
[GitHub] tinkerpop issue #801: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Github user PBGraff commented on the issue: https://github.com/apache/tinkerpop/pull/801 I'll modify and let you know when I'm done so that I make sure it's setup as I need. For my test I need not just a self-edge but also an edge to another vertex. ---
[GitHub] tinkerpop issue #801: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Github user PBGraff commented on the issue: https://github.com/apache/tinkerpop/pull/801 @spmallette : I believe this is ready now if you can run the Docker test: `docker/build.sh -t -n -i` ---
[GitHub] tinkerpop issue #801: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Github user PBGraff commented on the issue: https://github.com/apache/tinkerpop/pull/801 Never mind, I think I got it. ---
[GitHub] tinkerpop issue #801: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
Github user PBGraff commented on the issue:
https://github.com/apache/tinkerpop/pull/801
Hi @spmallette, I'm trying to add to the kitchen sink graph for my message
passing test and I want to make sure I'm doing it right. I'm doing this in Java
and calling a custom `main()`:
```
Graph g1 = TinkerGraph.open();
Graph g2 = TinkerGraph.open();
Graph g3 = TinkerGraph.open();
try {
// GraphSON v1.0
g1.io(GraphSONIo.build(GraphSONVersion.V1_0)).readGraph("../tinkerpop/data/tinkerpop-sink.json");
addData(g1);
g1.io(GraphSONIo.build(GraphSONVersion.V1_0)).writeGraph("tinkerpop-sink.json");
// GraphSON v2.0
g2.io(GraphSONIo.build(GraphSONVersion.V2_0)).readGraph("../tinkerpop/data/tinkerpop-sink-v2d0.json");
addData(g2);
g2.io(GraphSONIo.build(GraphSONVersion.V2_0)).writeGraph("tinkerpop-sink-v2d0.json");
// Gryo
g3.io(GryoIo.build()).readGraph("../tinkerpop/data/tinkerpop-sink.kryo");
addData(g3);
g3.io(GryoIo.build()).writeGraph("tinkerpop-sink.kryo");
} catch (IOException e) {
e.printStackTrace();
}
```
where
```
private static void addData(Graph g) {
Vertex a = g.addVertex(T.id, 1L, T.label, LABEL, PROPERTYIN, VTX_A);
Vertex b = g.addVertex(T.id, 2L, T.label, LABEL, PROPERTYIN, VTX_B);
a.addEdge(EDGE_LABEL, b);
a.addEdge(EDGE_LABEL, a);
}
```
`V1_0` appears to be coming out right, but `V2_0` looks to be typed when
the original isn't. Plus, I can't see how to read/write typed versions as
opposed to un-typed. Gryo appears to be straightforward. Should I add a GraphML
version?
---
[GitHub] tinkerpop pull request #801: TINKERPOP-1862 TinkerMessenger proper handling ...
GitHub user PBGraff opened a pull request: https://github.com/apache/tinkerpop/pull/801 TINKERPOP-1862 TinkerMessenger proper handling of Direction.BOTH New PR based off of #777 re-based on new head of tp32 branch. Need to update toy graph merged in #794 with scenario for this test and then update relevant tests. 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/801.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 #801 commit f02ea338a8d039c50896f7c65ca57a695975fc43 Author: Graff, Philip B Date: 2018-01-04T18:27:44Z TinkerMessenger proper handling of Direction.BOTH commit 80c0b8465fe9bb6ddbe4522a36304c7ba054e909 Author: Graff, Philip B Date: 2018-01-14T02:32:29Z Adding simple graph computer test of proper message passing in all 3 directions commit 750677cf55d7445c3746870d86c6592f6cd3664d Author: Graff, Philip B Date: 2018-01-14T04:49:27Z Test cleanup - hopefully can run now commit 153238b2a8a3fc5e051251662ef6ab80f72c659f Author: Graff, Philip B Date: 2018-01-14T06:01:17Z Additional check. Tests still fail - but I think this is a problem with the implementations, not the test. commit 5467a33f2e17ad22b736effebede124556f049ed Author: Graff, Philip B Date: 2018-01-14T18:44:33Z Test bug fix and refactoring repeated code ---
[GitHub] tinkerpop pull request #777: TINKERPOP-1862 TinkerMessenger proper handling ...
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...
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 #772: TINKERPOP-1861 Modify VertexProgram Builder to take va...
Github user PBGraff commented on the issue: https://github.com/apache/tinkerpop/pull/772 @spmallette That might have occurred with my attempt to rebase on the forked project. It ended up having to do a merge from my local into my remote branch, so those commits are probably (hopefully) already on the `tp32` branch. If it's really messed up, I can close this and apply the commits to a fresh branch off of the current `tp32`. The same goes for #770 . ---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
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...
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...
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 pull request #772: TINKERPOP-1861 Modify VertexProgram Builder to ...
Github user PBGraff commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/772#discussion_r162138049
--- Diff:
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/VertexProgram.java
---
@@ -219,13 +234,28 @@ public default void workerIterationEnd(final Memory
memory) {
* @paramThe vertex program type
* @return the newly constructed vertex program
*/
+@Deprecated
--- End diff --
Done in new commit. Will push with other changes.
---
[GitHub] tinkerpop issue #772: TINKERPOP-1861 Modify VertexProgram Builder to take va...
Github user PBGraff commented on the issue: https://github.com/apache/tinkerpop/pull/772 I think that sounds like a reasonable plan and I would appreciate it if you can cover part 4 on merge. I'll try to get to it soon. I understand that this can get more complicated since I am proposing an API change, which I wouldn't expect to be simple, especially for something more central. Thanks. ---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
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...
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 #772: TINKERPOP-1861 Modify VertexProgram Builder to take va...
Github user PBGraff commented on the issue: https://github.com/apache/tinkerpop/pull/772 I think all test problems have now been fixed. My concern is that people may still override/implement the old method and not the new one, thus yielding programs that don't execute properly. It will be indicated to them that they are using a deprecated method, however. ---
[GitHub] tinkerpop pull request #772: TINKERPOP-1861 Modify VertexProgram Builder to ...
Github user PBGraff commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/772#discussion_r161387759
--- Diff:
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/bulkloading/BulkLoaderVertexProgram.java
---
@@ -325,9 +325,12 @@ private Builder() {
@SuppressWarnings("unchecked")
@Override
-public BulkLoaderVertexProgram create(final Graph graph) {
-
ConfigurationUtils.append(graph.configuration().subset(BULK_LOADER_VERTEX_PROGRAM_CFG_PREFIX),
configuration);
-return (BulkLoaderVertexProgram)
VertexProgram.createVertexProgram(graph, configuration);
+public BulkLoaderVertexProgram create(final Graph... graphs) {
+if (graphs.length == 0) {
+throw new IllegalStateException("Must provide at least one
graph to use");
--- End diff --
Done.
---
[GitHub] tinkerpop pull request #772: TINKERPOP-1861 Modify VertexProgram Builder to ...
Github user PBGraff commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/772#discussion_r161387762
--- Diff:
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/VertexProgram.java
---
@@ -68,7 +68,20 @@ public default void storeState(final Configuration
configuration) {
* @param graph the graph that the VertexProgram will run
against
* @param configuration the configuration to load the state of the
VertexProgram from.
*/
+@Deprecated
--- End diff --
Thanks, done.
---
[GitHub] tinkerpop pull request #772: TINKERPOP-1861 Modify VertexProgram Builder to ...
Github user PBGraff commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/772#discussion_r161387755
--- Diff:
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/clustering/peerpressure/PeerPressureVertexProgram.java
---
@@ -81,7 +81,11 @@ private PeerPressureVertexProgram() {
}
@Override
-public void loadState(final Graph graph, final Configuration
configuration) {
+public void loadState(final Configuration configuration, final
Graph... graphs) {
+if (graphs.length != 1) {
+throw new IllegalStateException("Must provide one graph to
use, received " + graphs.length);
+}
+Graph graph = graphs[0];
--- End diff --
Done.
---
[GitHub] tinkerpop pull request #772: TINKERPOP-1861 Modify VertexProgram Builder to ...
Github user PBGraff commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/772#discussion_r161387752
--- Diff:
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/step/VertexComputing.java
---
@@ -53,7 +53,19 @@
* @param memory the {@link Memory} from the previous OLAP job if it
exists, else its an empty memory structure.
* @return the generated vertex program instance.
*/
-public VertexProgram generateProgram(final Graph graph, final Memory
memory);
+@Deprecated
--- End diff --
Done.
---
[GitHub] tinkerpop pull request #772: TINKERPOP-1861 Modify VertexProgram Builder to ...
Github user PBGraff commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/772#discussion_r161387754
--- Diff:
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/ranking/pagerank/PageRankVertexProgram.java
---
@@ -75,7 +75,11 @@ private PageRankVertexProgram() {
}
@Override
-public void loadState(final Graph graph, final Configuration
configuration) {
+public void loadState(final Configuration configuration, final
Graph... graphs) {
+if (graphs.length != 1) {
+throw new IllegalStateException("Must provide one graph to
use, received " + graphs.length);
+}
+Graph graph = graphs[0];
--- End diff --
Done.
---
[GitHub] tinkerpop pull request #772: TINKERPOP-1861 Modify VertexProgram Builder to ...
Github user PBGraff commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/772#discussion_r161387751
--- Diff:
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/step/map/PageRankVertexProgramStep.java
---
@@ -83,17 +83,17 @@ public String toString() {
}
@Override
-public PageRankVertexProgram generateProgram(final Graph graph, final
Memory memory) {
+public PageRankVertexProgram generateProgram(final Memory memory,
final Graph... graphs) {
final Traversal.Admin detachedTraversal =
this.edgeTraversal.getPure();
-
detachedTraversal.setStrategies(TraversalStrategies.GlobalCache.getStrategies(graph.getClass()));
+
detachedTraversal.setStrategies(TraversalStrategies.GlobalCache.getStrategies(graphs[0].getClass()));
--- End diff --
Since it's called through a step, I think you're right that there should
only be one `Graph`. I will validate in the "steps" that use the `Graph`.
---
[GitHub] tinkerpop pull request #772: TINKERPOP-1861 Modify VertexProgram Builder to ...
Github user PBGraff commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/772#discussion_r161387742
--- Diff:
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/computer/traversal/step/map/VertexProgramStep.java
---
@@ -64,15 +64,15 @@ public VertexProgramStep(final Traversal.Admin
traversal) {
if (this.first && this.getPreviousStep() instanceof EmptyStep)
{
this.first = false;
final Graph graph = this.getTraversal().getGraph().get();
-future =
this.getComputer().apply(graph).program(this.generateProgram(graph,
EmptyMemory.instance())).submit();
+future =
this.getComputer().apply(graph).program(this.generateProgram(EmptyMemory.instance(),
graph)).submit();
final ComputerResult result = future.get();
this.processMemorySideEffects(result.memory());
return
this.getTraversal().getTraverserGenerator().generate(result, this, 1l);
} else {
final Traverser.Admin traverser =
this.starts.next();
final Graph graph = traverser.get().graph();
final Memory memory = traverser.get().memory();
-future =
this.generateComputer(graph).program(this.generateProgram(graph,
memory)).submit();
+future =
this.getComputer().apply(graph).program(this.generateProgram(memory,
graph)).submit();
--- End diff --
I'll revert this change.
---
[GitHub] tinkerpop issue #777: TINKERPOP-1862 TinkerMessenger proper handling of Dire...
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 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 Date: 2018-01-04T18:27:44Z TinkerMessenger proper handling of Direction.BOTH commit 01a9ee5ae80fd429726d0722edae3195d9a625c5 Author: Graff, Philip B Date: 2018-01-14T02:32:29Z Adding simple graph computer test of proper message passing in all 3 directions ---
[GitHub] tinkerpop pull request #770: TINKERPOP-1862 TinkerMessenger proper handling ...
Github user PBGraff closed the pull request at: https://github.com/apache/tinkerpop/pull/770 ---
[GitHub] tinkerpop pull request #772: TINKERPOP-1861 Modify VertexProgram Builder to ...
GitHub user PBGraff opened a pull request: https://github.com/apache/tinkerpop/pull/772 TINKERPOP-1861 Modify VertexProgram Builder to take varargs Graphs VertexProgram.Builder.create() now takes varargs Graphs instead of just a single Graph. This change has been propagated through all methods affected. Old API is kept but deprecated for interfaces. Implementations of these interfaces have been updated to use the new API. Possible test problems that I don't understand and build flags due to API change. You can merge this pull request into a Git repository by running: $ git pull https://github.com/PBGraff/tinkerpop TINKERPOP-1861 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/772.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 #772 commit 2a121f5a18385432c55000626114561a371ba138 Author: Graff, Philip B Date: 2018-01-08T21:26:05Z First set of changes moving to new API commit 48cb3dd3214050f7b54f16fa3576ec36f1196c49 Author: Graff, Philip B Date: 2018-01-08T20:22:48Z Updates to keep old API but deprecated commit ba2236dbfe85447265a8e56cabac46e419f665d1 Author: Graff, Philip B Date: 2018-01-08T21:16:34Z Modifying the test ---
[GitHub] tinkerpop pull request #768: TINKERPOP-1861 Modify VertexProgram.Builder.cre...
Github user PBGraff closed the pull request at: https://github.com/apache/tinkerpop/pull/768 ---
[GitHub] tinkerpop pull request #770: TINKERPOP-1862 TinkerMessenger proper handling ...
GitHub user PBGraff opened a pull request: https://github.com/apache/tinkerpop/pull/770 TINKERPOP-1862 TinkerMessenger proper handling of Direction.BOTH This PR presents a fix for `TinkerMessenger` that handled `Direction.BOTH` by identifying the `Vertex` that is not the one receiving the message. In the case of self-loops, the same `Vertex` is still returned so these are handled properly. Note that self-loops cause messages to be sent/received twice since the `Edge` is both `IN` and `OUT`. 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/770.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 #770 commit 70b74de41a288ea27f0cc6b89cba57d07c3bc3d0 Author: Graff, Philip B Date: 2018-01-04T18:27:44Z TinkerMessenger proper handling of Direction.BOTH ---
[GitHub] tinkerpop issue #768: TINKERPOP-1861 Modify VertexProgram.Builder.create() t...
Github user PBGraff commented on the issue: https://github.com/apache/tinkerpop/pull/768 @spmallette We can apply this to the TP 3.2.x line as well. Should I modify the PR to apply to the tp32 branch instead? ---
[GitHub] tinkerpop pull request #768: WIP: Modify VertexProgram.Builder.create() to t...
GitHub user PBGraff opened a pull request: https://github.com/apache/tinkerpop/pull/768 WIP: Modify VertexProgram.Builder.create() to take Graph as varargs VertexProgram.Builder.create() now takes varargs Graph instead of just Graph. This change has been propagated through all methods affected. WIP since build fails due to API changes. Javadocs might need additional small modifications. You can merge this pull request into a Git repository by running: $ git pull https://github.com/PBGraff/tinkerpop TINKERPOP-1861 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/768.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 #768 commit 3e102646788bbe0dfa83e79700c8591ea6cb8d45 Author: Graff, Philip B Date: 2018-01-03T20:36:22Z VertexProgram.Builder.create() now takes varargs Graph instead of just Graph. This change has been propagated through all methods affected. ---
