incubator-clerezza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Henry Story <henry.st...@bblfish.net>
Subject Re: delay in release
Date Thu, 14 Jul 2011 12:21:14 GMT

On 14 Jul 2011, at 12:35, Reto Bachmann-Gmür wrote:

> I agree with the principle of getting things to work first and then
> care about performance but here I think it would be a fundamentally
> flaw in the architecture that will necessarily cause very bad
> performance.

I was seeing it not as a flaw in the architecture, but rather an optimisation problem.
Let me develop that line of thinking. 

First: 
    would the following not solve all the problems in the test suite?
Replace

   graph.addAll(sub.getGraph)

with

   if (graph != sub.getGraph) 
          graph.addAll(sub.getGraph)

   Then none of the code written in the test suite would lead to copying of
graphs, since they are always built from the the same EzMGraph.

Second, (perhaps the following is the missing justification)

    Relating a node in one graph to a bnode in another does not make sense
unless you also copy some context over. Ok, here I copy all the graph: perhaps 
copying the smallest context would be enough (though that will change depending 
on what type of inference is running). It's an interesting issue as to what is
easiest to understand. Copying all over may be easiest to understand, and then 
providing a method to produce the minimum graph would be an option, so that the
developer can decide how much he really needs.

But the main point is below:

> One can get the test in the issue (which really if a
> request for enhancement) to pass without introducing these parts.

   I don't think that the tests would pass if you don't add those changes too. The reason
the code in your
tests works with constructs such as 

  reto.a(FOAF.Person)
           -- FOAF.knows --> (
		"http://bblfish.net/#hjs".uri.a(FOAF.Person)
			-- FOAF.name --> "Henry Story"
			-- FOAF.currentProject --> "http://webid.info/".uri
                )

   is because the following happens (starting from the center)

  1.  "http://bblfish.net/#hjs".uri.a(FOAF.Person) 
     
   a)   gets transformed into
   
         new URIRef("http://bblfish.net/#hjs")

      because the .uri method is defined on EzLiteral and there is the 
      implicit def string2lit(str: String) = new EzLiteral(str)

   b)  then it gets transformed into a RichGraphNode - let's call it anon_rgn_hjs - because
the method a(..) is defined there and because of the implicit (call it IMP)

       implicit def toRichGraphNode(resource: Resource) = {
		new RichGraphNode(new GraphNode(resource, baseTc))
	} 

   2. Then the RichGraphNode anon_rgn_hjs is filled out with the FOAF.name and FOAF.currentProject
relations
   
   3. reto - a rich graph node - is then related by foaf knows to anon_rgn_hjs.resource
        by the following function

		def -->(sub: GraphNode): RichGraphNode = {
			//RichGraphNode.this + sub
			-->(sub.getNode)
		}


    QUESTION: what happens to the graph of anon_rgn_hjs??? How does it get added to the graph
of reto?
    -------- 

   Well this occurs because of the magic you were using, which this whole issue is questioning
the wisdom of,
namely the implicit we mentioned above and named IMP:

      implicit def toRichGraphNode(resource: Resource) = {
		new RichGraphNode(new GraphNode(resource, baseTc))
      } 

    Notice that the implicit created a new GraphNode, whose graph is called "baseTc". But
where does baseTc come from?
Ahah. IMP is part of a trait:

protected trait TcDependentConversions extends TcIndependentConversions {
	
	def baseTc: TripleCollection
	
	implicit def toRichGraphNode(resource: Resource) = {
		new RichGraphNode(new GraphNode(resource, baseTc))
	}
}

And  EzMGraph is defined as extending that trait.

     class EzMGraph(val baseTc: MGraph) extends AbstractMGraph with TcDependentConversions


putting it together:
-------------------

When you create a new EzMGraph - call it ez - you create an object with the toRichGraphNode()
implicit, whose baseTc set to that Graph's tc collection. The you

   import ez._ 

to import that implicit so that it becomes available for usage. Of course that implicit is
tightly tied to ez. 
So this explains then how anon_rgn_hjs gets added to ez. It's because when in 1b above when

new URIRef("http://bblfish.net/#hjs") was transformed into a RichGraphNode using the IMP implicit,
the backing
graph of ez was added as its backing store.

Ok so all that is really clever! But just a bit too clever by half, sadly.

Remember that the point of this patch is to fix the real problem with the current framework,
which is that
currently if you have two graphs you need to keep writing the import statements every time
you wish to 
change graph. Like this:

   import ez1._ 
   ez1.b_("reto") -- FOAF.homepage --> "http://bblfish.net/".uri 

   import ez2._ 
   ez2.b_("hjs") -- FOAF.homepage --> "http://bblfish.net/".uri 

   import ez1._ 
   ez1.b_("reto") -- FOAF.currentProject --> "http://clerezza.org/".uri 
 

This is very error prone it seems to me.

And the reason you have that error, is because you have created an implicit that is tied
to a hidden variable. Now I really am willing to bet some money that if you put this to the

Scala mailing list they will award you with the discovery of a Scala anti-pattern :-)


	Henry


> 
> Reto
> 
> On Thu, Jul 14, 2011 at 12:23 PM, Henry Story <henry.story@bblfish.net> wrote:
>> 
>> On 14 Jul 2011, at 11:32, Reto Bachmann-Gmür wrote:
>> 
>>> The issue I can agree with addressing is 603. While I think the naming
>>> is inappropriately dramatic as it would be better described as
>>> supporting a slightly different coding style, as I commented on the
>>> issue I could agree with a resolution.
>>> 
>>> Not sure how to apply your patch, it doesn't seem to work with the
>>> regular path -p (see below).
>> 
>> I don't know. That is what they recommend to do on
>> 
>>   http://www.apache.org/dev/git.html
>> 
>> I agree that  they should also give some info on how to apply the patch!
>> 
>>> 
>>> I think the changes in the patch pertinent to the issue are the
>>> changes to the bnode and the b_ methods, I don't see how other changes
>>> like
>>> 
>>>               def -->(sub: GraphNode): RichGraphNode = {
>>> -                     //RichGraphNode.this + sub
>>> +                     graph.addAll(sub.getGraph)
>>>                       -->(sub.getNode)
>>>               }
>> 
>> If you have want code like
>> 
>>   gr.bnode("danny") -- FOAF.knows --> ( gr.bnode("reto").a(FOAF.Person) ... )
>> 
>> to work then you need to allow the --> to take a GraphNode as object. It should
of course merge the
>> info.
>> 
>> If you want to make it more efficient, you could open another issue to make it more
efficient. A simple
>> test of equality between the graphs would certainly help.
>> 
>> 
>> 
>>> 
>>> or
>>> 
>>> +
>>> +             /**
>>> +              * Add one relation for each member of the iterable collection
>>> +              */
>>> +             def -->>>(elems: Iterable[GraphNode]): RichGraphNode =
{
>>> +                     for (res <- elems) -->(res)
>>> +                     RichGraphNode.this
>>> +             }
>> 
>> This is just so that you can allow relations to lists of GraphNodes. Sadly it can't
be the same as -->> as that clashes
>> with the other methods due to Type Erasure.
>> 
>>> 
>>> would relate to the issue. (And as a side-note, I don't think a
>>> potentially very expensive operation like copying around the whole
>>> context of a GraphNode should happen as a side effect of the -->
>>> method)
>> 
>> I think it is better to file a new issue to make the code more efficient. There are
different ways to do that I am sure, so that could itself give rise to a debate. No need to
mix issues. Simplicity is the mantra now. Right ? :-)
>> 
>> Henry
>> 
>> 
>> 
>>> 
>>> Reto
>>> 
>>> 
>>> Attempt to apply the patch:
>>> reto@reto-HPE-540ch:~/projects/apache/clerezza/trunk/parent$ wget
>>> https://issues.apache.org/jira/secure/attachment/12486181/0001-CLEREZZA-603-EzMGraph.bnode-now-returns-a-RichGraphN.patch
>>> --2011-07-14 11:14:53--
>>> https://issues.apache.org/jira/secure/attachment/12486181/0001-CLEREZZA-603-EzMGraph.bnode-now-returns-a-RichGraphN.patch
>>> Resolving issues.apache.org... 140.211.11.121
>>> Connecting to issues.apache.org|140.211.11.121|:443... connected.
>>> HTTP request sent, awaiting response... 200 OK
>>> Length: 10792 (11K) [text/plain]
>>> Saving to: `0001-CLEREZZA-603-EzMGraph.bnode-now-returns-a-RichGraphN.patch'
>>> 
>>> 100%[===============================================================================================>]
>>> 10,792      --.-K/s   in 0s
>>> 
>>> 2011-07-14 11:14:54 (21.5 MB/s) -
>>> `0001-CLEREZZA-603-EzMGraph.bnode-now-returns-a-RichGraphN.patch'
>>> saved [10792/10792]
>>> 
>>> reto@reto-HPE-540ch:~/projects/apache/clerezza/trunk/parent$ patch -p0
>>> < 0001-CLEREZZA-603-EzMGraph.bnode-now-returns-a-RichGraphN.patch
>>> patching file b/parent/platform.security.foafssl/test/src/main/scala/org/apache/clerezza/foafssl/test/WebIDTester.scala
>>> Hunk #1 FAILED at 33.
>>> Hunk #2 FAILED at 122.
>>> Hunk #3 FAILED at 130.
>>> Hunk #4 FAILED at 148.
>>> Hunk #5 FAILED at 172.
>>> Hunk #6 FAILED at 199.
>>> Hunk #7 FAILED at 215.
>>> Hunk #8 FAILED at 223.
>>> Hunk #9 FAILED at 602.
>>> Hunk #10 FAILED at 709.
>>> Hunk #11 FAILED at 743.
>>> 11 out of 11 hunks FAILED -- saving rejects to file
>>> b/parent/platform.security.foafssl/test/src/main/scala/org/apache/clerezza/foafssl/test/WebIDTester.scala.rej
>>> patching file b/parent/rdf.scala.utils/src/main/scala/org/apache/clerezza/rdf/scala/utils/EzMGraph.scala
>>> Hunk #1 FAILED at 54.
>>> Hunk #2 FAILED at 72.
>>> 2 out of 2 hunks FAILED -- saving rejects to file
>>> b/parent/rdf.scala.utils/src/main/scala/org/apache/clerezza/rdf/scala/utils/EzMGraph.scala.rej
>>> patching file b/parent/rdf.scala.utils/src/main/scala/org/apache/clerezza/rdf/scala/utils/Preamble.scala
>>> Hunk #1 FAILED at 105.
>>> 1 out of 1 hunk FAILED -- saving rejects to file
>>> b/parent/rdf.scala.utils/src/main/scala/org/apache/clerezza/rdf/scala/utils/Preamble.scala.rej
>>> patching file b/parent/rdf.scala.utils/src/main/scala/org/apache/clerezza/rdf/scala/utils/RichGraphNode.scala
>>> Hunk #1 FAILED at 225.
>>> 1 out of 1 hunk FAILED -- saving rejects to file
>>> b/parent/rdf.scala.utils/src/main/scala/org/apache/clerezza/rdf/scala/utils/RichGraphNode.scala.rej
>>> patching file b/parent/rdf.scala.utils/src/test/scala/org/apache/clerezza/rdf/scala/utils/EzMGraphTest.scala
>>> Hunk #1 FAILED at 111.
>>> Hunk #2 FAILED at 120.
>>> Hunk #3 FAILED at 142.
>>> 3 out of 3 hunks FAILED -- saving rejects to file
>>> b/parent/rdf.scala.utils/src/test/scala/org/apache/clerezza/rdf/scala/utils/EzMGraphTest.scala.rej
>>> reto@reto-HPE-540ch:~/projects/apache/clerezza/trunk/parent$ svn status
>>> ?       b
>>> ?       0001-CLEREZZA-603-EzMGraph.bnode-now-returns-a-RichGraphN.patch
>>> 
>>> On Thu, Jul 14, 2011 at 11:05 AM, Henry Story <henry.story@bblfish.net>
wrote:
>>>> Since there is a delay in the release I think it would be worth considering
a little
>>>> bit more closely the patches I submitted recently:
>>>> 
>>>> - CLEREZZA-599 - EzMGraph imports too many implicits
>>>> - CLEREZZA-600 - remove need to define Preamble in code using rdf.scala.utils
>>>> - CLEREZZA-603 - EzMGraph fails when working with more than one instance
>>>> 
>>>> These are all very small patches I filed as requested. Each one is very easy
to understand and self
>>>> contained. CLEREZZA-603 is the one that makes the usefulness of the others
even clearer than what
>>>> they are by themselves.
>>>> 
>>>>   Henry Story
>>>> 
>>>> Social Web Architect
>>>> http://bblfish.net/
>>>> 
>>>> 
>> 
>> Social Web Architect
>> http://bblfish.net/
>> 
>> 

Social Web Architect
http://bblfish.net/


Mime
View raw message