[Tinkerpop3] GraphStrategy design alternative

173 views
Skip to first unread message

Dave

unread,
Dec 30, 2014, 8:45:09 PM12/30/14
to gremli...@googlegroups.com
Hi Stephen Mallette,

After coming back from a week's vacation, trying to code some custom strategy methods requiring chaining caused me to look at the Tinkerpop source code and triggered some ideas. I took the existing source and with minimal modification was able to implement a different design for graph strategies that made chaining a lot easier, still kept boilerplate to a minimum and provided easy access to the core graph implementation elements.

Essentially,I came up with a hybrid of Tinkerpop 2 and Tinkerpop 3 implementations - every strategy uses its own set of strategy-wrapped element instances (StrategyGraph, StrategyVertex etc.). That is, an element is wrapped in layers of strategy-wrapped classes that are mapped one-to-one to strategies. Obviously it is more memory intensive but I've been converting all of my strategies to the revised implementation and have been quickly able to achieve what I've been trying to do and have found it easier to use. So, I wanted to share it with you to see what you thought (and perhaps if it is of interest to others using strategies).


Here are the changes I made to the tinkerpop source, which are trivial given the extent of the conceptual design change:-

1) I deleted the code in the StrategyXX classes that throws an exception if a wrapped instance is passed into the constructor. This allowed me to wrap a strategy element in another strategy element - each layer pointing to a different strategy.

    DELETED: if (baseGraph instanceof StrategyWrapped) throw new IllegalArgumentException(
                                  String.format("The graph %s is already StrategyWrapped and must be a base Graph", baseGraph));


2) I added the concept of "inner elements" and "getInnerXXX" methods. That is, the ability to "peel" back a strategy layer to get to the "inner" layer. I also changed the getBaseXXX method as follows:-

    MODIFIED: public Graph getBaseGraph() {
                           if (getInnerGraph() instanceof StrategyWrapped)
                               return ((StrategyGraph)getInnerGraph()).getBaseGraph();    // recursively finds the base element
                           else
                               return getInnerGraph();
                      }

    ADDED: public Graph getInnerGraph() {
                     return this.baseGraph;               //   Note that I didn't change the name of the "baseXX" variable passed into the constructor. For readability it should be named "innerXX".
                 }



3) For the standard method implementations calling the strategy methods in the StrategyXX classes I changed the "getBaseXX::YY" call to "getInnerXX::YY" such as:-

    @Override
    public Vertex addVertex(final Object... keyValues) {
        final Optional<Vertex> v = Optional.ofNullable(compose(
                s -> s.getAddVertexStrategy(this.graphContext, strategy),
==>                getInnerGraph()::addVertex).apply(keyValues));
        return v.isPresent() ? new StrategyVertex(v.get(), this) : null; . 



That's pretty much it - all the changes were to the StrategyXX classes. I didn't change the Graph.strategies() method even though it probably won't work now. I now instantiate strategies more representative of M6 and prior:-

        Graph tinkerGraph = TinkerGraph.open();

        strategy = new AuditStrategy()
        auditGraph = new StrategyGraph(tinkerGraph, strategy);

        strategy = new ModelStrategy(modelVertex);
        modelGraph() = new StrategyGraph(auditGraph, strategy);


The other benefit to this is that I can override the StrategyGraph class to provide methods that access the underlying strategy methods, making them feel like first class citizens. And since Strategy classes can wrap other strategy classes chaining strategies and calling methods on the underling elements is relatively easy (see getInnerVertex() below) :-

    public UnaryOperator<Supplier<Void>> getRemoveVertexStrategy(final StrategyContext<StrategyVertex> ctx, final GraphStrategy composingStrategy) {
        return (f) -> () -> {
==>         final Vertex innerVertex = ctx.getCurrent().getInnerVertex();
            innerVertex.property(Constants.REMOVED, System.currentTimeMillis());
            return null;
        };
    }


I have forked the Tinkerpop 3 source. I am 2-3 days out from completing coding and testing. Once completed I will check it in if you want to look at it. Also, please let me know your thoughts and if it is useful since I don't know how others are using strategies. It does satisfy all of my requirements that I've mentioned in our discussions. 

Regards,
Dave
 





Stephen Mallette

unread,
Dec 31, 2014, 6:30:14 AM12/31/14
to gremli...@googlegroups.com
Interesting work.  I had mostly blocked off allowing a StrategyGraph from wrapping a StrategyGraph because I wasn't sure if there would be problems in complex scenarios.  As you have a complex scenario and didn't have problems then I guess there's no real issue.  The rest I don't see a problem with.  Please let me know when you have your fork up to date.  I'd like to see your changes in there.

--
You received this message because you are subscribed to the Google Groups "Gremlin-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to gremlin-user...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/gremlin-users/9d3935b7-6f9e-4337-8f79-c8eb8d9d3cc5%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Dave

unread,
Dec 31, 2014, 1:01:46 PM12/31/14
to gremli...@googlegroups.com
Hi Stephen,

I just committed the basic changes to the StrategyXXX classes to my own fork:-


1) In the StrategyGraph class I was unsure whether to use the base graph or inner graph for the following methods:- of(), compute(), tx(), variables(), configuration(), features(). I suspect it is a mixture of both but this definitely needs to be reviewed. I have not tested against them.
2) I don't use the "composingStrategy" parameter but decided to leave it in. I think composing strategies will still work as long as the caller does not attempt to pass in nested strategy wrappers, which may break it.
3) I left the "Graph.strategies()" method in. I suspect it will still work but have not tested it. Also, I do not use it for instantiating nested strategy graphs. 

Essentially the design conceptually incorporates nested wrappers like Tinkerpop 2 but leverages the strategy concept from TInkerpop 3 to eliminate the boilerplate. So far its working for me but I have not thoroughly tested nested strategies (I don't expect any problems) or put any thought into an elegant way to instantiate a sequence of strategies.

Regards,
Dave

Stephen Mallette

unread,
Dec 31, 2014, 1:50:40 PM12/31/14
to gremli...@googlegroups.com
please see below:

Thanks for doing that.  Doesn't look like too heavy a set of changes.  
 
1) In the StrategyGraph class I was unsure whether to use the base graph or inner graph for the following methods:- of(), compute(), tx(), variables(), configuration(), features(). I suspect it is a mixture of both but this definitely needs to be reviewed. I have not tested against them.

This was another part of my reason for not wanting to nest the StrategyGraph - complications in this area seem to abound and I didn't want to mess up.  My sense is that if you nest the expectation should be that we always call "inner" and never bypass to go to "base". 
 
2) I don't use the "composingStrategy" parameter but decided to leave it in. I think composing strategies will still work as long as the caller does not attempt to pass in nested strategy wrappers, which may break it.

I'm glad you didn't change that - I want to prefer composition to nesting.  I am a little worried about having multiple ways to do the same thing.  I want to avoid questions on the mailing list like, "When should i nest and when should i compose?" 
 
3) I left the "Graph.strategies()" method in. I suspect it will still work but have not tested it. Also, I do not use it for instantiating nested strategy graphs. 

Essentially the design conceptually incorporates nested wrappers like Tinkerpop 2 but leverages the strategy concept from TInkerpop 3 to eliminate the boilerplate. So far its working for me but I have not thoroughly tested nested strategies (I don't expect any problems) or put any thought into an elegant way to instantiate a sequence of strategies.

Please keep us posted on your progress with this work especially with respect to your testing efforts.   It would be nice to know that you were able to accomplish what you wanted with this approach.  Perhaps, opening this door to nesting doesn't create a lot of problems and we can clearly explain when one should use it over composition.  I'm still must not be completely clear on why composition doesn't work for you.  I wish we could reduce your issue to a test case (i thought that i had) so that it was something I could tinker on to get it to work (or confirm that nesting was the only way).  



Dave

unread,
Dec 31, 2014, 3:32:23 PM12/31/14
to gremli...@googlegroups.com
HI Stephen,

Please see below. Also, I think the bottom line is that strategies should be promoted as reusable modules that can be easily "plugged in" to enhance a graph's core functionality (how many times has versioning been mentioned on the message board?). This will inevitably require a disciplined approach when developing a (generic) strategy to ensure it plays nicely with others. That is, except for the specialized strategies, a strategy should assume it will be embedded in a sequence and hence pass it's calls "down the line" and clearly document when it doesn't (and why). 

Lastly, I know you have standard JUnit test harnesses for the strategy wrappers. What do I need to do to setup my environment to use the standard tests against my modifications?   I do have Maven installed but only just started using it to build the Tinkerpop source i.e. I'm a newbie.    I am also on Windows and have been running Maven with -DskipTests because some tests (not related to Strategy) were failing. Additionally, I would like to test my custom WrappedGraph class against the core API tests. I'm assuming the preferred method is to override some standard test methods for my custom cases. Is this correct?  Where do I start?

Thanks,
Dave

On Wednesday, December 31, 2014 10:50:40 AM UTC-8, Stephen Mallette wrote:
please see below:


Thanks for doing that.  Doesn't look like too heavy a set of changes.  

Yes, very straight forward and only the "StrategyXXX" classes are minimally affected.  

 
 
1) In the StrategyGraph class I was unsure whether to use the base graph or inner graph for the following methods:- of(), compute(), tx(), variables(), configuration(), features(). I suspect it is a mixture of both but this definitely needs to be reviewed. I have not tested against them.

This was another part of my reason for not wanting to nest the StrategyGraph - complications in this area seem to abound and I didn't want to mess up.  My sense is that if you nest the expectation should be that we always call "inner" and never bypass to go to "base". 

After giving it some thought I agree - everything should reference "inner" instead of "base" even though in some cases the implementation might always defer to the underlying base. 
 
2) I don't use the "composingStrategy" parameter but decided to leave it in. I think composing strategies will still work as long as the caller does not attempt to pass in nested strategy wrappers, which may break it.

I'm glad you didn't change that - I want to prefer composition to nesting.  I am a little worried about having multiple ways to do the same thing.  I want to avoid questions on the mailing list like, "When should i nest and when should i compose?" 
 

I agree with your concerns about multiple methods for extending, especially with regard to questions on the mailing list but I do think that they are slightly different tools and have their place. 

3) I left the "Graph.strategies()" method in. I suspect it will still work but have not tested it. Also, I do not use it for instantiating nested strategy graphs. 

Essentially the design conceptually incorporates nested wrappers like Tinkerpop 2 but leverages the strategy concept from TInkerpop 3 to eliminate the boilerplate. So far its working for me but I have not thoroughly tested nested strategies (I don't expect any problems) or put any thought into an elegant way to instantiate a sequence of strategies.

Please keep us posted on your progress with this work especially with respect to your testing efforts.   It would be nice to know that you were able to accomplish what you wanted with this approach.  Perhaps, opening this door to nesting doesn't create a lot of problems and we can clearly explain when one should use it over composition.  I'm still must not be completely clear on why composition doesn't work for you.  I wish we could reduce your issue to a test case (i thought that i had) so that it was something I could tinker on to get it to work (or confirm that nesting was the only way).  

I think the composition method would have possibly worked for me but it was definitely getting cumbersome when chaining strategies - a lot of "brain-lifting". Calling, for example getInnerGraph().addVertex() method is very simple and intuitive since it aligns with the core Graph API versus the methods in the GraphStrategy interface e.g. getAddVertexStrategy(). I'm currently modifying the Graph.strategies() method to return a StrategyGraph that includes inner strategy graphs that wrap the list of strategies. This will hide the nesting to some extent but it also allows the developer to extend StrategyGraph to, among other things, provide their own custom methods to access the inner classes. And, a developer can also nest strategies in SequenceStrategy and pass the sequenced strategy into Graph.strategies(), which utilizes the M6 way of instantiation.   

Stephen Mallette

unread,
Jan 1, 2015, 12:09:00 PM1/1/15
to gremli...@googlegroups.com
Lastly, I know you have standard JUnit test harnesses for the strategy wrappers. What do I need to do to setup my environment to use the standard tests against my modifications?   I do have Maven installed but only just started using it to build the Tinkerpop source i.e. I'm a newbie.    

running "mvn clean install" is the most generic answer to that question.  if you want to run the tests from your IDE then you will need to run an implementation test like TinkerGraphStructureStandardTest (which is in the tinkergraph-gremlin project).
 
I am also on Windows and have been running Maven with -DskipTests because some tests (not related to Strategy) were failing.

i'm dusted off my windows vm and did get a few build failures when running tests.  i issued a commit that fixed the problem for me:


still needs some cleanup, but note that i was able to do a "mvn clean install" with this configuration:

> mvn -version
Apache Maven 3.2.5 (12a6b3acb947671f09b81f49094c53f426d8cea1; 2014-12-14T12:29:23-05:00)
Maven home: C:\java\apache-maven-3.2.5
Java version: 1.8.0_25, vendor: Oracle Corporation
Java home: C:\Program Files\Java\jdk1.8.0_25\jre
Default locale: en_US, platform encoding: Cp1252
OS name: "windows 7", version: "6.1", arch: "amd64", family: "dos"
 
Additionally, I would like to test my custom WrappedGraph class against the core API tests. I'm assuming the preferred method is to override some standard test methods for my custom cases. Is this correct?  Where do I start? 

If you want to a run a the full test suite over your WrappedGraph, then you need to treat your wrapper like a regular implementation:


The key to getting this to work is to override the features() method so that the tests execute given the capabilities of your wrapper.  Given the dynamic nature of strategy composition I've simply had to conceded that overriding features just isn't really possible so there is now "strategy" method for them.  However, given that you have your own wrapper, i guess you could override that method directly.  

Dave

unread,
Jan 2, 2015, 4:35:08 PM1/2/15
to gremli...@googlegroups.com
Hi Stephen,

Thanks for this information.  Testing the strategy wrappers against the full test suite is my next step but probably it won't happen for a little while. 

I just uploaded some changes and bug fixes to my fork for the strategy wrapper classes. I modified the Graph.strategies() method to automatically generate the inner wrappers for a list of strategies and return the outer wrapped graph - the signature remained the same. Passing strategies and sequenced strategies works like in M6 but I haven't tested it yet. It is feasible to have a wrapper contain a sequence of strategies and itself being wrapped in outer wrappers that contain other strategies (even sequences). 

I have 98% of my code working including multiple nested wrappers and it supports everything that was in our discussions. It was fairly painless - boilerplate was kept to a minimum without losing flexibility. The only thing that was a gotcha was that I created a custom outer wrapper (inherits from StrategyGraph) that exposes the inner strategies so that I can easily access their custom strategy methods. This seems innocent but the gotcha is that some of my custom strategy methods accept a StrategyXXX class (typically StrategyVertex) and hence it is possible to pass an outer StrategyXXX instance to the strategy method for an inner class, which caused things to get out of sync (can be a tricky bug to find). I am thinking about providing a mechanism that peels back layers of strategy wrappers (maybe a helper) to ensure that it is in sync with the strategy method it is being passed to. Any ideas on this would be appreciated. Please let me know what you think.

Thanks,
Dave

Stephen Mallette

unread,
Jan 5, 2015, 7:40:15 AM1/5/15
to gremli...@googlegroups.com
If nested StrategyGraph instances are really working well, then I wonder if we should simply remove SequenceStrategy.  Nested StrategyGraph instances are simple to understand, whereas SequenceStrategy requires a very functional way of thinking.  Unless there is a clear reason why one would use SequenceStrategy instead of nesting I'm hesitant to keep it and include nesting at the same time.  ???

I think a positive thing with doing nested instead of SequenceStrategy is that Graph.features() can be a strategy method which enables a StrategyGraph to have meaning programmatically (i.e. you can reason against the features of a StrategyGraph).  The nice thing about that capability is that we could have a whole line of testing that put all the GraphStrategy implementations under the full test suite, thus validating their behavior.  

I'm assuming that you would like to see nesting included in the TP3 code base.  Note that if you would like to offer a pull request from your fork, you will need to sign the TP3 CLA (via http://clahub.com) prior to doing so. Once you do that, then we can work together to get your initial pull request merged - from there we can collaborate on how to further harden the approach in preparation for M7.  Please let me know if that's how you would like to proceed.




To view this discussion on the web visit https://groups.google.com/d/msgid/gremlin-users/fd000a9e-c0c9-4e8b-8607-0d8bb9122069%40googlegroups.com.

Dave

unread,
Jan 5, 2015, 4:52:40 PM1/5/15
to gremli...@googlegroups.com
Hi Stephen,

Comments below.

Also, I had problems building the gremlin-test module from my fork since it was missing a guava dependency. Last night I downloaded the latest snapshot and found that this issue was fixed. However, when doing a "mvn clean install" from the root directory I received a number of "illegal working directory" errors (below). Note that there is a space in my absolute directory name where the repository is:-  "D:\My Documents\ITC\Repositories\tinkerpop3> "


g_v1_out_pathXage_nameX(com.tinkerpop.gremlin.process.graph.step.map.GroovyPathTest$StandardTest)  Time elapsed: 0 sec  <<< ERROR!
java.lang.IllegalArgumentException: The workingDirectory is not a directory or does not exist
        at com.tinkerpop.gremlin.structure.io.kryo.KryoReader$Builder.workingDirectory(KryoReader.java:384)
        at com.tinkerpop.gremlin.AbstractGraphProvider.readIntoGraph(AbstractGraphProvider.java:106)
        at com.tinkerpop.gremlin.AbstractGraphProvider.loadGraphData(AbstractGraphProvider.java:64)
        at com.tinkerpop.gremlin.AbstractGremlinTest.setup(AbstractGremlinTest.java:113)
        at sun.reflect.GeneratedMethodAccessor9.invoke(Unknown Source)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:483)
        at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
        at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
        at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
        at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:24)
        at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
        at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:55)
        at org.junit.rules.RunRules.evaluate(RunRules.java:20)
        at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)
        at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
        at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
        at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
        at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
        at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
        at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
        at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
        at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
        at org.junit.runners.Suite.runChild(Suite.java:127)
        at com.tinkerpop.gremlin.AbstractGremlinSuite.runChild(AbstractGremlinSuite.java:183)
        at com.tinkerpop.gremlin.AbstractGremlinSuite.runChild(AbstractGremlinSuite.java:34)
        at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
        at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
        at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
        at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
        at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
        at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
        at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:264)
        at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:153)
        at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:124)
        at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:200)
        at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:153)
        at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:103)



I also setup tests per the docs as for a new graph vendor. I ran the tests against my graph strategy graphs using TInkerGraph as the base graph and also received similar errors below (I used TinkerGraph's "Features" and "OptIn" settings). Please note that I am running against my forked Tinkerpop3 EXCEPT for gremlin-test, which is the (buildable) version I downloaded last night. However, even if my errors are because of this it doesn't explain the errors from building the snapshot above. 


java.lang.IllegalArgumentException: The workingDirectory is not a directory or does not exist
at com.tinkerpop.gremlin.structure.io.kryo.KryoReader$Builder.workingDirectory(KryoReader.java:384)
at com.tinkerpop.gremlin.AbstractGraphProvider.readIntoGraph(AbstractGraphProvider.java:106)
at com.tinkerpop.gremlin.AbstractGraphProvider.loadGraphData(AbstractGraphProvider.java:64)
at com.tinkerpop.gremlin.AbstractGremlinTest.setup(AbstractGremlinTest.java:113)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:24)
at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
at com.carrotsearch.junitbenchmarks.BenchmarkStatement$BaseEvaluator.evaluateInternally(BenchmarkStatement.java:104)
at com.carrotsearch.junitbenchmarks.BenchmarkStatement$SequentialEvaluator.evaluate(BenchmarkStatement.java:148)
at com.carrotsearch.junitbenchmarks.BenchmarkStatement.evaluate(BenchmarkStatement.java:389)
at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:55)
at org.junit.rules.RunRules.evaluate(RunRules.java:20)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:77)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:56)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
at org.junit.runners.Suite.runChild(Suite.java:128)
at org.junit.runners.Suite.runChild(Suite.java:27)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
at org.junit.runners.Suite.runChild(Suite.java:128)
at com.tinkerpop.gremlin.AbstractGremlinSuite.runChild(AbstractGremlinSuite.java:183)
at com.tinkerpop.gremlin.AbstractGremlinSuite.runChild(AbstractGremlinSuite.java:34)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:67)


I am on Windows 7.1. Here is my environment:-

Apache Maven 3.2.3 (33f8c3e1027c3ddde99d3cdebad2656a31e8fdf4; 2014-08-11T13:58:10-07:00)
Maven home: d:\apache-maven-3.2.3\bin\..
Java version: 1.8.0_25, vendor: Oracle Corporation
Java home: c:\java\jdk1.8.0_25\jre
Default locale: en_US, platform encoding: Cp1252
OS name: "windows 7", version: "6.1", arch: "amd64", family: "dos"


Lastly, in the future I assume you'd rather me create an Issue on Github versus post on the user's group?
 
Regards,
Dave


On Monday, January 5, 2015 4:40:15 AM UTC-8, Stephen Mallette wrote:
If nested StrategyGraph instances are really working well, then I wonder if we should simply remove SequenceStrategy.  Nested StrategyGraph instances are simple to understand, whereas SequenceStrategy requires a very functional way of thinking.  Unless there is a clear reason why one would use SequenceStrategy instead of nesting I'm hesitant to keep it and include nesting at the same time.  ???


I would have to agree with you about removing SequenceStrategy. Although, you were pretty bullish on chaining strategies functionally versus nested wrappers but I'm not sure what your reasons were - I'm not sure what sequenced strategies can do that nested wrappers can't (besides using less memory). I personally will always lean to the nested wrappers because coding with them has been a lot easier to understand  and debug (specifically chaining) along with being able to treat a StrategyGraph instance as a first class citizen.

 
I think a positive thing with doing nested instead of SequenceStrategy is that Graph.features() can be a strategy method which enables a StrategyGraph to have meaning programmatically (i.e. you can reason against the features of a StrategyGraph).  The nice thing about that capability is that we could have a whole line of testing that put all the GraphStrategy implementations under the full test suite, thus validating their behavior.

Yes!!!  Also community-contributed strategies  :-)  

 
 

I'm assuming that you would like to see nesting included in the TP3 code base.  Note that if you would like to offer a pull request from your fork, you will need to sign the TP3 CLA (via http://clahub.com) prior to doing so. Once you do that, then we can work together to get your initial pull request merged - from there we can collaborate on how to further harden the approach in preparation for M7.  Please let me know if that's how you would like to proceed.



Yes please. I will down load the CLA and return ASAP. 

Dave

unread,
Jan 5, 2015, 5:36:12 PM1/5/15
to gremli...@googlegroups.com
Hi Stephen,

I have one thing that I would like your opinion on regarding strategy implementation. When developing a strategy implementation I came across behavior that took me a little while to debug and thought others could fall into the same trap. My "audit" strategy marked an element as "removed" rather than removing it and my iterators simply did not return the element if it was "removed". The problem I came across was when trying to retrieve the value for a property. I saw that the default behavior in the base interfaces is:-  

- Vertex.property(key) uses the property iterator to find the property
- Element.value(key) calls element.property(key)

Because of this I only implemented the property iterator strategy method and not "vertex.property(key)" or "element.value(key)" i.e. I relied on the default behavior in the interfaces to take care of the rest. Lean code - great!!  The problem was that TinkerGraph overrode the property and value methods and threw multiple-value errors (remember I don't delete an element, just mark it as removed). 

The concern is that if only the minimum strategy methods are implemented in a strategy, the strategy could break depending on which underlying base graph was used. The remedy is to implement the strategy methods to mimic the default behavior in the base interfaces but now you're introducing copying/pasting boilerplate, which is what TP3 was suppose to help reduce. 

I guess a graph strategy could optionally inherit from a DefaultStrategy class???  What do you think of this approach?   Secondly, since the behavior is already implemented in the base interfaces, is there an elegant way to leverage this instead of inheriting from a default strategy class?

Thanks,
Dave
 
...

Stephen Mallette

unread,
Jan 6, 2015, 7:12:57 AM1/6/15
to gremli...@googlegroups.com
Hi Dave, please see below...

Also, I had problems building the gremlin-test module from my fork since it was missing a guava dependency. Last night I downloaded the latest snapshot and found that this issue was fixed. However, when doing a "mvn clean install" from the root directory I received a number of "illegal working directory" errors (below). Note that there is a space in my absolute directory name where the repository is:-  "D:\My Documents\ITC\Repositories\tinkerpop3> "


g_v1_out_pathXage_nameX(com.tinkerpop.gremlin.process.graph.step.map.GroovyPathTest$StandardTest)  Time elapsed: 0 sec  <<< ERROR!
java.lang.IllegalArgumentException: The workingDirectory is not a directory or does not exist

I've just pushed some changes that make that error message a little nicer.  The test should also create the directory if it doesn't exist.  Finally, the working directory has been moved under the target folder so that it better cleans up after itself.  Please pull those changes to see if that fixes anything.
 
I also setup tests per the docs as for a new graph vendor. I ran the tests against my graph strategy graphs using TInkerGraph as the base graph and also received similar errors below (I used TinkerGraph's "Features" and "OptIn" settings). Please note that I am running against my forked Tinkerpop3 EXCEPT for gremlin-test, which is the (buildable) version I downloaded last night. However, even if my errors are because of this it doesn't explain the errors from building the snapshot above. 


java.lang.IllegalArgumentException: The workingDirectory is not a directory or does not exist

Let's see what new errors you get once you pull the latest changes.

Lastly, in the future I assume you'd rather me create an Issue on Github versus post on the user's group?

Let's reserve the issue tracker for bugs and specific design issues that need discussion.  I don't think we're annoying anyone yet with our GraphStrategy talk :)
 
I would have to agree with you about removing SequenceStrategy. Although, you were pretty bullish on chaining strategies functionally versus nested wrappers but I'm not sure what your reasons were - I'm not sure what sequenced strategies can do that nested wrappers can't (besides using less memory). I personally will always lean to the nested wrappers because coding with them has been a lot easier to understand  and debug (specifically chaining) along with being able to treat a StrategyGraph instance as a first class citizen.

I think we should drop SequenceStrategy in favor of your approach - there's clearly use cases where the nesting is needed and if it does everything that SequenceStrategy did then there isn't much point to keeping that.  Let's just make sure we get some good tests in place around this nesting approach so that there is the same level of confidence in the approach.
 
I'm assuming that you would like to see nesting included in the TP3 code base.  Note that if you would like to offer a pull request from your fork, you will need to sign the TP3 CLA (via http://clahub.com) prior to doing so. Once you do that, then we can work together to get your initial pull request merged - from there we can collaborate on how to further harden the approach in preparation for M7.  Please let me know if that's how you would like to proceed.

Yes please. I will down load the CLA and return ASAP. 

you should not need to "download" the CLA - you should be able to just sign into http://clahub.com with your GitHub account and electronically sign - it just takes a few moments.  You can even revoke privileges to clahub after you sign as they have ask for a lot of permissions they really don't need.  Feel free to upvote this issue to fix that: https://github.com/clahub/clahub/issues/54 

Marko and I were planning on releasing M7 early next week - is there any way we can wrap up your changes (and the CLA) in the next couple of days so that we can stay in line with that schedule?

Thanks,

Stephen
 

Stephen Mallette

unread,
Jan 6, 2015, 7:23:00 AM1/6/15
to gremli...@googlegroups.com
This is definitely a problem.  The "helper" methods (default implementations on interfaces) that we introduce in gremlin-core can be overridden thus creating question as to where strategy implementers should inject their code.  We've added the @Graph.Helper annotation that blocks vendors from overriding some of our defaults.  In that way we can avoid creating GraphStrategy methods for those because they can be intercepted by implementing "lower" GraphStrategy methods.  This approach was making full and total sense until we hit the very methods you are talking about (and others).  

I wish the rule were as simple as if it is a "default" method then it gets @Helper, but there are times when that might not be so good (e.g. what if a Graph implementation could optimize the performance of a method over the default?).  

Anyway, I've asked Marko to give some thought to the issue at this point:


Please fee free to include your thoughts.

--
You received this message because you are subscribed to the Google Groups "Gremlin-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to gremlin-user...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/gremlin-users/b98c94da-7b51-4921-bb37-ef5289adbd5e%40googlegroups.com.

Dave

unread,
Jan 7, 2015, 3:57:12 AM1/7/15
to gremli...@googlegroups.com
Hi Stephen,

A few things:-

1) I will visit the Gihub website and get the CLA taken care of on Wednesday
2) The strategy wrapper code in my fork is ready to go except for the issue I mentioned above about the defaults in the interfaces being overridden,
3) Regarding the issue of defaults being overridden, how about adding code to the Graph Strategy methods that mimics their base counterparts?    For example. if the default Element.value(key) contains Element.property(key).value() then the default getVertexValueStrategy() should do the same - call getVertexGetPropertyStrategy(). It duplicates coding but I can't think of a more elegant way to do it.
4) In Github how do I pull commits from the main Tinkerpop3 repository into my fork?   

Thanks,
Dave
  
...

Stephen Mallette

unread,
Jan 7, 2015, 9:49:50 AM1/7/15
to gremli...@googlegroups.com
> 3) Regarding the issue of defaults being overridden, how about adding code to the Graph Strategy methods that mimics their base counterparts?    For example. if the default Element.value(key) contains Element.property(key).value() then the default getVertexValueStrategy() should do the same - call getVertexGetPropertyStrategy(). It duplicates coding but I can't think of a more elegant way to do it.

That's an interesting idea - i think that could work.  Could you try it out in your fork and see how that works for you?

> 4) In Github how do I pull commits from the main Tinkerpop3 repository into my fork? 

--
You received this message because you are subscribed to the Google Groups "Gremlin-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to gremlin-user...@googlegroups.com.

Dave

unread,
Jan 7, 2015, 3:38:49 PM1/7/15
to gremli...@googlegroups.com
Hi Stephen,

Firstly, I "signed" the CLA. Please let me know if I missed anything. Secondly, I tried providing "default" implementations for some of the methods in the GraphStrategy interface without success. The problem is how the strategy wrapper classes are written. Here is StrategyVertex.value():-

    public <V> V value(final String key) throws NoSuchElementException {
        return this.strategyGraph.compose(
                s -> s.<V>getVertexValueStrategy(this.strategyContext, strategy),
                this.getInnerVertex()::value).apply(key);
    }


Assuming I haven't missed anything the problem is the call to getInnerVertex().value. It makes this call on the inner vertex, which in the case where it is the last strategy in a chain would call the method on the base vertex, which provides the alternate/unwanted behavior. Here are some options:-

1) remove "dependent" methods from the strategy classes  (e.g. StrategyVertex.value()), which would force the default methods in the base interfaces to be called. However, then the strategy developer cannot enhance these methods. This may require a need to allow a developer to inject their own class that inherits the strategy wrapper class and override the specific method  (I'm actually looking at this for another use case).

2) Pass the alternate method into the StrategyGraph.compose() method  e.g. pass property(key)::value instead of getInnerVertex()::value.  But I'm not as intimate with the code as you are and don't know if passing a method with a different signature but the same parameter types would break something (or if it would even compile).  

I unfortunately am limited on time over the next couple of days to explore this further. However, all the code I've developed to-date has been checked into my fork. 

Thanks,
Dave
...

Dave

unread,
Jan 13, 2015, 11:12:20 AM1/13/15
to gremli...@googlegroups.com
Hi Stephen,

1)  The GraphStrategy interface contains a lot of methods with similar names. A few times now I have caught myself incorrectly implementing the wrong strategy method as well as trying to debug the wrong method. What do you think about nested interfaces to organize the methods according to their base counterparts  e.g. GraphStrategy.Graph, GraphStrategy.Vertex, GraphStrategy.Edge etc. ?

2) With the strategy wrapper approach I think it now makes more sense to support and expose all the methods from the base interfaces (Graph, Vertex etc) as strategy methods. We've been caught using property.IsPresent() in a sequence that introduced bugs because IsPresent() does not have a strategy counterpart so the call was passed to the base graph. This could also be a problem if someone tries to use the packaged strategies with their own. For example, lets say I use PartiionStrategy that sits before my own AuditStrategy (it time-stamps elements and never deletes an element - just marks it as removed). If a property is created at t0, removed at t1 and then queried at t2, the wrong result may be returned because PartitionGraph calls IsPresent, which will always returns true because it's called on the base graph.

Please let me know what you think. I'm also happy to provide some time to help implement. 

Regards,
Dave
P.S. why is SubgraphStrategy now not used as the parent to others e.g. PartitionStrategy?    It is marked as final. Some of our own strategies inherited from it. 

  

...

Stephen Mallette

unread,
Jan 14, 2015, 6:30:26 AM1/14/15
to gremli...@googlegroups.com
answers inline below:

1)  The GraphStrategy interface contains a lot of methods with similar names. A few times now I have caught myself incorrectly implementing the wrong strategy method as well as trying to debug the wrong method. What do you think about nested interfaces to organize the methods according to their base counterparts  e.g. GraphStrategy.Graph, GraphStrategy.Vertex, GraphStrategy.Edge etc. ?

yeah - it feels a bit out of hand - we could probably do something like that going after M7 is released.
 
2) With the strategy wrapper approach I think it now makes more sense to support and expose all the methods from the base interfaces (Graph, Vertex etc) as strategy methods. We've been caught using property.IsPresent() in a sequence that introduced bugs because IsPresent() does not have a strategy counterpart so the call was passed to the base graph. This could also be a problem if someone tries to use the packaged strategies with their own. For example, lets say I use PartiionStrategy that sits before my own AuditStrategy (it time-stamps elements and never deletes an element - just marks it as removed). If a property is created at t0, removed at t1 and then queried at t2, the wrong result may be returned because PartitionGraph calls IsPresent, which will always returns true because it's called on the base graph.

Marko and I still haven't addressed this issue yet with respect to the @Helper annotation - though your example sounds like a solid case for having to do all the methods.  Let's address this as soon as M7 is released.  I think we've all learned enough about GraphStrategy at this point to harden and finalize the approach for GA.  Your continued help in getting us there would be appreciated.
 
P.S. why is SubgraphStrategy now not used as the parent to others e.g. PartitionStrategy?    It is marked as final. Some of our own strategies inherited from it. 

We found some problems in extending PartitionStrategy from it somewhere along the line.  Unmarking as final is probably ok - i think that was in your recent PR. 


--
You received this message because you are subscribed to the Google Groups "Gremlin-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to gremlin-user...@googlegroups.com.
Reply all
Reply to author
Forward
0 new messages