[PEAK] A few issues and suggestions

[Phillip J. Eby]

At 11:08 PM 11/3/2008 +0200, Sergey Schetinin wrote:
>1. A small change to circularity detection to make errors easier to interpret
>
>In Controller._retry:
>
>                 if item in self.routes:
>                     path = find_circularity(item, self.routes, item, {})
>                     if path:
>                         raise CircularityError(path)
>
>
>def find_circularity(item, routes, start, seen):
>     for via in routes.get(item, ()):
>         if via is start:
>             return (via,)
>         elif via not in seen:
>             seen[via] = 1
>             path = find_circularity(via, routes, start, seen)
>             if path is not None:
>                 return path + (via,)

I decided to include both the routes and the path, since there may be 
more than one circularity, and this path detection only shows one of them.


>2. I had an issue when there's no circularity, but no valid evaluation
>order either. The rules weren't conflicting, but depending on order
>rules ran the dependency tree was different, so the retry was an
>infinite loop, cycling through a set of different possible rule
>evaluation orders. This is a rare case, and I cannot immediately write
>a test to reproduce it. It can be detected: if a certain order of
>scheduled rules was already tried in this transaction, that means we
>are in infinite loop. But this detection is rarely needed and would be
>relatively costly, so if ever implemented, I think should not kick in
>until there was a fair number of retries in a single transaction,
>which would indicate we are possibly in an infinite loop.
>
>Why I'm mentioning this is because I didn't think this case was
>possible, but I do see it consistently in one big test app, which
>would be quite hard to strip down to make it a test case. Maybe you'll
>have better luck constructing a small set of rules to reproduce it,
>but I'll try it again in a few days anyway.

I think the simple thing here would be just to add a sanity check for 
the total number of retries, and perhaps some sort of retry log.  I 
won't be implementing that today, though.


>3. Assigning Cell instances to CellAttributes.
>When you set a component attribute to a cell (for ex. c.val = Cell())
>it's treated as a special case and overwrites the cell in __cells__
>dictionary. I suppose this is done to simplify use of existing cells
>in component initialization, but there seem to be no other use for it.

I was actually planning to (eventually) go more the other way, and 
allow CellAttributes to treat an assignment of a cell as triggering a 
changed() notice to the old cell underneath, after first replacing it.

>However it's useful sometimes to have a cell as a value of another
>cell, so it would be nice if it this special case was implemented in
>Component.__init__ instead of CellAttribute.__set__

Note that nothing stops you from making the outer cell a regular attribute.


>4. Also related, new @compute rules being settable, which I mentioned
>in http://www.eby-sarna.com/pipermail/peak/2008-October/003116.html
>There's a special case to support that, but that doesn't make any
>sense to me. What is it for?

The initialization case is to allow overriding a readonly rule or 
constant at construction time, while preserving the otherwise 
read-only nature of the attribute.  However, the second case is a 
(small) loophole: unless the cell has been used at least once, there 
is still a window during which you can set the attribute.

Without a more specific reason for changing this, I don't see it as a 
problem; the main goal of the trellis is to have a consistent view of 
things, and it is a perfectly viable consistency strategy to allow a 
value to be set so long as it hasn't been read by anything yet.  In 
this case, it will also become immutable upon its first writing, so 
it is even more strict than is necessary to preserve consistency and 
the DAG-ness of the program.