Remove add-tokens! and remove-tokens! calls from ProductionNode #386
Comments
|
Thanks for the thorough and clear explanation. I’ve built a considerable amount of tooling on top of rule matches. To me they’re magic and one of the great things about Clara because it lets us know basically not just what happened but why. My implementation uses the listeners namespace so I’m not sure this change would affect that. I’m not sure if there’s test coverage that would show breakage in that namespace directly or if it’d only surface indirectly in the inspect tests. |
If I'm understanding Will's proposed removal of memory interaction in the ProductionNode correctly, this change wouldn't affect the information that is accessible via Clara's listeners. The listener calls are left intact. The change seems to only affect anyone who was directly accessing the memory of the ProductNode via the memory API. This is a lower-level of access and wouldn't be something that we'd expect to be stable to rely on as an external API anyways. However, Clara did use this memory access in its own implementation of session inspection that Will linked to above. So to not change inspection output, that'd have to be changed to figure out the So Alex, I'm not sure if you are using the |
|
@mrrodriguez Thanks. Wasn't sure but wanted to be sure :) Looks like I'm getting |
|
@alex-dixon In general my approach to the changes to inspection would be that it is "beta enough" that some changes to the way the data is accessed are probably reasonable depending on the case in question, but that I don't want to actually change what data is made available. Basically perhaps a minor change in consumer codebases when taking a new version of Clara, but not anything that would undercut assumptions about what data is available and thus user's architectures. I don't want to go around completely breaking people's code. :) Obviously opinions may vary on what degree of change is acceptable. I'll post some more detailed thoughts later, but one approach could be that if certain operations require additional information the user could be required to attach a new "inspection listener" provided by Clara to the session. Do note that we have the information on rule matches for cases where logical insertions resulted elsewhere too, and other fields in the inspection data map are essentially different projections of this information. |
|
@alex-dixon However, from your code above, I don't know how you are obtaining these tokens. Will's changes here would result in ProductionNode to not be directly adding/removing tokens of their own since these tokens aren't useful at leaf nodes in the rete graph. If you had something relying on getting the tokens for a given ProductionNode, then you may be affected. The listener calls though shouldn't be affected. I'd think there would be no reason to remove the listener activate/retract (eg. If you are getting tokens more directly via hitting the Clara memory API for tokens by node-ids etc, then this sort of change could have an affect on you. |
|
Regarding :rule-matches in clara.tools.inspect, its behavior seems somewhat odd to me at present. Take the following case I'd expect it to return a :rule-match in this case: but not in this case: The reason I expect this is that the tokens are removed from the beta-memory upon retraction regardless of what happens in the RHS and session inspection uses the beta memory tokens to generate :rule-matches data. However, if my rule is performing unconditional insertions, RHS retractions, or otherwise executing side effects what I really would care about is that the rule was in fact fired, not that its support was removed, since the removal of that support would not remove the impact of the activation on the state of the system. This makes me think that a more functionally correct/useful approach might be to:
Thoughts? |
So you are proposing to leave out any unconditional fact details from this functionality now?
There is already a listener on the left-activate in the ProductionNode. This can already be used to monitor all activity of the rule regardless of the truth maintenance. |
Correct. I don't think the current behavior makes sense in that it excludes unconditional fact details if the support is retracted, which isn't meaningful in the case of unconditional insertions. I'd consider the current behavior to be broken due to this.
I'm envisioning an API like the following: where :all-rule-matches would have the same information as the current :rule-matches key, but for all rule matches that led to a RHS firing regardless of whether the match was retracted. Along the same lines you could create an :unconditional-fact->explanations key, which I believe has been requested on either the Slack channel or mailing list before. Basically I'd like to make to make all the current information available from session inspection still available and actually increase it (by showing rule activations whose support was retracted). The user would just need to make a call to add an additional listener implementation supplied by Clara. The doc on such a listener would make clear that it holds onto references that would otherwise be GC'ed. At the same time, if the user doesn't need the additional information they could choose to take the performance benefits of not keeping track of it. Note that this is distinct from saying that the user has the ability to create their own listener, which is a more difficult thing to do. I think this might require adding some new listener call(s) to new listener method(s) around activation firing. |
|
@WilliamParker why are activation listeners special compared to the existing listener protocol? I find it odd that it comes from the inspect namespace, which is a higher-level than the listeners APIs. I just am worried the API is getting clunky with this. Listeners weren't generally specific to inspection. Also, inspection was fine without any supplied user-level listeners before. The ideal scenario is probably: (1) If we could remove the support for However, I think this is likely problematic for people. I've seen several clara-in-cljs impl's using unconditional inserts and RHS retracts, mostly due to Clara not providing a clear way to do a fact-update right now. Losing that inspection may be fairly impactful. (2) The current listener API could have support for activation listening. One problematic thing as far as the existing listener protocolis concerned though, is that it isn't very extensible. There isn't a good way to add new functions to it and not break existing impl's. (3) If a listener is in use that implements this new activation listening, it would be used by inspection. This would be different than existing inspection features though, since the rest are ok without custom-listener impl's. |
I'm just not sure that the current listener methods provide the granularity of "was this RHS actually fired" versus "was this ProductionNode left-activated". Regarding the concerns about breaking those using :rule-matches on unconditional insertions now, regardless of the performance improvement discussed here, it seems to me that they
I agree that needing to add a listener to access certain subsets of session inspection functionality is a bit clunky, but it is the best option I've found so far that allows us to:
The reason to add it to session inspection would be to reduce the clunkiness as much as possible - the user wouldn't even need to understand the idea of listeners. It could simply be documented as "enable more inspection on this session at the cost of holding onto more memory". Obviously we'd want to very clearly communicate the change when releasing a version of Clara with the change. Regarding the addition of new listener methods, to be honest my POV on that is that if one is going to create a custom implementation of a low-level Clara API one should reasonably expect that it may be necessary to update it from time to time to take new versions of Clara. I'd be more hesitant to remove listener methods that are currently present, but when adding new method(s) such a consumer can just add an empty implementation of them. It is a very easy uplift to perform. |
What does it currently return? Unconditional facts with no support? I may need to run your above examples since you discussed your expectations, but not the current behavior (perhaps we should add them to this post).
Yeah, I definitely wasn't suggesting to hold references to facts that aren't currently held. Not a viable option.
Perhaps this functionality should instead go in
I don't know that I agree here. The listener API has a lot of utility for external purposes. I'm wondering if Clara could provide an extension mechanism that wouldn't need to be broken after each new feature is added to it, i.e. as new listener functionality is added. Removing things is a different topic than adding. That is often harder to be compatible with. That should be really rare though in this case. I don't think it is too frequent to expect some fundamental shift in fact/token propagation in the engine that must break the existing listener interface. |
|
I wrote out an example of a case I think is broken now below. This was done in a REPL against the master branch. Basically, if you're using session inspection to understand how the Second fact came to exist in the session, these cases should both return the same result, which they don't currently do. I suppose you could argue that the :rule-matches "correctly" returns rule matches that weren't removed, but if you care about unconditional operations I don't see how that is useful, and suspect that users just aren't taking that caveat into account. |
|
@WilliamParker thanks for the details. I agree it is confusing to have those 2 different behaviors depending on if retract happened or not. After a retract the fact no longer has an explanation. Doesn't sound intuitive to me. |
More precisely, the :rule-match map is as if the rule never fired at all; the empty entry you're seeing would be present in any case i.e. it is a map of the rule to an empty list of matches. Also notably, the :fact->explanations keys doesn't have unconditionally inserted facts since its implementation uses mem/get-insertions-all and mem/get-insertions which both use the production-memory. The production-memory is the field in the memory that is used for truth maintenance; see here in the engine for example. This was a deliberate design decision since we don't want the overhead of keeping track of facts in this way for some performance-intensive cases; thus unconditional insertions can be an "escape hatch" to more performant procedural code. The :insertions keys won't have unconditionally inserted facts for the same reason. Demonstration: I believe there have been requests for inspection functionality on unconditional insertions before, but it has never been implemented. Inspection functionality backed by a listener would allow us to do this. Something like The reason I'd be inclined against putting it in tracing is that the fact that it would use a listener would be an implementation detail - the user would only really need to be concerned with the tradeoff of the additional functionality versus holding onto references. I'd also like to clearly communicate that tracing is a less stable API than session inspection, and mixing things of varying stability in the same namespace seems more confusing to me. |
Thanks for the clarification.
That sounds reasonable. I see inspection features as more flexible and easier to toggle different behaviors on via functions like you propose. I think that listener API may be used more for consumers to perform logic that is important for their own logic's correctness. So my biggest concern I'd have at this point would be doing something to the listeners where they no longer had enough information to find facts that were unconditionally inserted/retracted. |
I don't see any reason to remove the l/left-activate! or l/left-retract! calls from ProductionNode. Since the default listener will just have a null operation the performance cost should be very minimal. We can just add a comment on why the listener calls exists without a corresponding memory call i.e. for passivity. EDIT: Link to the listener calls, not the memory calls as referenced by @mrrodriguez below. |
I think you meant the listener calls and not the memory calls you linked right? e.g. Removing the memory token modifications was the optimization proposed by this issue I think. |
Oops. You are correct @mrrodriguez . I corrected the comment. |
|
OK, so to summarize the proposed changes: Inspection API Changes
The inspect/full-activation-logging function would return a new session rather than modifying the session passed to it.
Something similar could be done for a counterpart to the :insertions key as well.
Listener changes
Performance benefits
Breakage of consumers
Individual notes:
|
|
I've merged a PR to address this issue: #387 Closing. |
Summary:
We appear to be making calls to the memory in the ProductionNode that we don't actually need and which can just be removed without replacement, but with some complications related to session inspection. The performance benefits of this on rules firing and serialization/deserialization times could be significant.
Longer explanation:
The ProductionNode is a leaf node in a Clara Rete-style rules network that essentially corresponds to the RHS of a rule. This node is responsible for adding RHS activations to a queue to be activated and retracting logically inserted facts when their support is removed.
For understanding the discussion below some brief review of the fields on the TransientLocalMemory is recommended. The TransientLocalMemory is used during rules firing.
Queueing the RHS activations is done by this call to mem/add-activations!. Note that the body of this method mutates the "activation-map" field of the memory. When the engine reaches a point at which it will execute a queued RHS activation, it will call mem/pop-activation!, which also mutates the activation-map. When the support for a RHS activation is removed before it has been executed, the engine will call mem/remove-activations! which also operates on the activation-map. In short, all necessary information on what RHS activations are currently in the queue is contained within the activation map and accessed through it.
When a ProductionNode needs to remove logically inserted facts because their support was retracted, it calls mem/remove-insertions! in order to determine what exactly needs to be retracted. The remove-insertions! implementation accesses the "production-memory" field of the memory and subsequently mutates it. When the engine performs a logical insertion, it [calls mem/add-insertions!] to keep track of what logical insertions have been performed. The implementation of mem/add-insertions! likewise changes the production-memory. In short, the production-memory contains all necessary information to keep track of logical insertions and retract them when their support is removed.
However, the ProductionNode also calls mem/add-tokens! upon left-activation and mem/remove-tokens! upon left-retract. Both mem/add-tokens! and mem/remove-tokens! act on the beta-memory field of the memory. Also note that the ProductionNode does not use the results of either the mem/add-tokens! or mem/remove-tokens! call.
It appears to me that the calls to mem/add-tokens! and mem/remove-tokens! in the ProductionNode are not actually necessary for any function of the engine. The code paths don't seem to lead to any outcome. Furthermore, the activation-map keeps track of the RHS queued activations in full, as does the production-memory for logical insertions. The ProductionNode does lose access to information about previously executed RHS activations that didn't cause any logical insertions. However, the ProductionNode has no actual work to perform if the support for such an activation is removed, so I don't believe this is actually a problem. For example, such a RHS activation might have triggered only unconditional insertions, in which case nothing will be retracted even when the support is removed. Since ProductionNode is a leaf node it doesn't need to keep track of retractions to pass downstream to its (nonexistent) descendants.
The only place that seems to actually use the tokens kept in the beta-memory field for a ProductionNode is session inspection, specifically the generation of :rule-matches data. This implementation takes a simple approach of retrieving all tokens from each ProductionNode and then generating explanations for them. However, I believe that it should be possible for the inspection namespace to rebuild this information from the immediate parent nodes of each ProductionNode. Alternatively, perhaps we don't actually care about returning this information, but no longer doing so would be a nonpassive change to session inspection.
When I simply removed this line in left-activate and this line in left-retract all tests passed in Clojure except tests of session inspection. I didn't run ClojureScript tests locally since my ClojureScript environment is having problems.
The text was updated successfully, but these errors were encountered: