[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 spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/772 ok - i'll just close this then. if you change your mind, we can get it reopened and work it through. 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 #772: TINKERPOP-1861 Modify VertexProgram Builder to take va...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/772 @PBGraff do you think you will come back to this PR to get it finished up? or is that unlikely given your schedule? ---
[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 #772: TINKERPOP-1861 Modify VertexProgram Builder to take va...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/772 Sorry - you really shouldnt have any commits from our repo in this PR - it should be all your work. The problem is that if when we look at the diff to review, it contains changes that don't pertain to your work, which makes the changes impossible to evaluate by themselves. I imagine you must have merged tp32 into the branch in your fork or something. You should have instead rebased your local changes onto our tp32 which would have re-wrote all of your commits on top of ours so that the git history made it look like you had all your changes on the head of tp32. Hope that makes sense. If you need some more git explanation, please let me know. ---
[GitHub] tinkerpop issue #772: TINKERPOP-1861 Modify VertexProgram Builder to take va...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/772 > 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 Hmmas I look at the way it is now, you mention a valid concern. This change effectively breaks existing `VertexProgram` implementations, doesn't it? Meaning, nothing in TinkerPop calls the deprecated method anymore, so existing `VertexProgram` implementations will no longer have the old method called at runtime - it will call the empty default method instead here: https://github.com/apache/tinkerpop/pull/772/files#diff-ca108f45ec7b5a288dfce8019f1a6813L72 Perhaps the right way to do this is to: 1. Make the new method call the deprecated one by default. In that way, existing `VertexProgram` implementations just work on upgrade. Leave the deprecated one empty. 2. Our `VertexProgram` implementations implement the new method, so they will all work without a problem. The old deprecated method should be implemented on these classes to call the new method. This way, if someone (for some weird reason) is relying on the behavior of the old method it will still work without change (you would need to add appropriate deprecation annotation/javadoc here. 3. People who write new `VertexProgram` implementations wouldn't bother with the old deprecated method - perhaps some stronger language in the javadoc would make that clear to them. 3. Write [upgrade docs](https://github.com/apache/tinkerpop/blob/tp32/docs/src/upgrade/release-3.2.x-incubating.asciidoc) to describe the new expectations around `VertexProgram` implementations and include a changelog entry - i can do that if you like on merge. How does that sound? btw, sorry - this is turning into a complicated PR - thanks for continuing to work through this with us. ---
[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 issue #772: TINKERPOP-1861 Modify VertexProgram Builder to take va...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/772 actually - i see that there is a whole other host of errors: ```text [ERROR] Errors: [ERROR] org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest.shouldFailWithImproperTraverserRequirements(org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest) [INFO] Run 1: PASS [ERROR] Run 2: GraphComputerTest.shouldFailWithImproperTraverserRequirements:2448 » NullPointer [ERROR] Run 3: GraphComputerTest.shouldFailWithImproperTraverserRequirements:2448 » NullPointer [INFO] Run 4: PASS [INFO] Run 5: PASS [INFO] [ERROR] org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest.shouldSucceedWithProperTraverserRequirements(org.apache.tinkerpop.gremlin.process.computer.GraphComputerTest) [INFO] Run 1: PASS [ERROR] Run 2: GraphComputerTest.shouldSucceedWithProperTraverserRequirements:2426 » NullPointer [ERROR] Run 3: GraphComputerTest.shouldSucceedWithProperTraverserRequirements:2426 » NullPointer [INFO] Run 4: PASS [INFO] Run 5: PASS [INFO] [ERROR] GroovyProgramTest$Traversals>ProgramTest.g_V_outXcreatedX_aggregateXxX_byXlangX_groupCount_programXTestProgramX_asXaX_selectXa_xX:117 » IllegalState [ERROR] org.apache.tinkerpop.gremlin.process.traversal.step.map.ProgramTest$Traversals.g_V_outXcreatedX_aggregateXxX_byXlangX_groupCount_programXTestProgramX_asXaX_selectXa_xX(org.apache.tinkerpop.gremlin.process.traversal.step.map.ProgramTest$Traversals) [ERROR] Run 1: ProgramTest$Traversals>ProgramTest.g_V_outXcreatedX_aggregateXxX_byXlangX_groupCount_programXTestProgramX_asXaX_selectXa_xX:117 » IllegalState [ERROR] Run 2: ProgramTest$Traversals>ProgramTest.g_V_outXcreatedX_aggregateXxX_byXlangX_groupCount_programXTestProgramX_asXaX_selectXa_xX:117 » IllegalState [ERROR] Run 3: ProgramTest$Traversals>ProgramTest.g_V_outXcreatedX_aggregateXxX_byXlangX_groupCount_programXTestProgramX_asXaX_selectXa_xX:117 » IllegalState [ERROR] Run 4: ProgramTest$Traversals>ProgramTest.g_V_outXcreatedX_aggregateXxX_byXlangX_groupCount_programXTestProgramX_asXaX_selectXa_xX:117 » IllegalState [ERROR] Run 5: ProgramTest$Traversals>ProgramTest.g_V_outXcreatedX_aggregateXxX_byXlangX_groupCount_programXTestProgramX_asXaX_selectXa_xX:117 » IllegalState ``` Are you still looking into a resolution for these? ---
[GitHub] tinkerpop issue #772: TINKERPOP-1861 Modify VertexProgram Builder to take va...
Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/772 I add a bunch of comments - pretty minor changes mostly. The biggest thing I'm seeing right now comes from the failing Travis build: ```text [ERROR] /home/travis/build/apache/tinkerpop/spark-gremlin/src/main/java/org/apache/tinkerpop/gremlin/spark/process/computer/traversal/strategy/optimization/SparkInterceptorStrategy.java:[47,112] cannot find symbol symbol: method getTraversal() location: interface org.apache.tinkerpop.gremlin.process.computer.VertexProgram [ERROR] /home/travis/build/apache/tinkerpop/spark-gremlin/src/main/java/org/apache/tinkerpop/gremlin/spark/process/computer/traversal/strategy/optimization/SparkSingleIterationStrategy.java:[64,112] cannot find symbol symbol: method getTraversal() location: interface org.apache.tinkerpop.gremlin.process.computer.VertexProgram ``` Is that the problem you were alluding to in the PR description? ---
