Pass node, token arguments to all listener methods

[alex-dixon]

Previous discussion here:
#307 (comment)

Will update with latest master, squash and write tests. Just wanted to open to show proposed changes and start discussion.

[mrrodriguez]

One comment I have so far is that it is a breaking change to any implementations of the affected listener protocols.

[alex-dixon]

@mrrodriguez Heh. Oops.

Would multiple arity dispatch and preserving argument order on the protocols address that sufficiently?

(retract-facts! [listener facts])
(retract-facts! [listener facts node token])

Calling from within engine:

(when listener
 -      (l/retract-facts! listener facts))		 +      (l/retract-facts! listener node token facts))

[mrrodriguez]

@alex-dixon I believe the syntax is

(retract-facts! ([listener facts]) ([listener facts node token]))

I'd have to double check.

Besides that though, I don't think it is so easy to "expand protocols" like this. If we immediately start calling the higher-arity, old impl's would fail with no implementation exceptions.

It may be the case (would have to check again) that Clara is already setting up an intermediary listener that delegates to the callers given listener(s). If that is the case, we could consider using the delegating listener as a "proxy" to the underlying that somehow avoids calling a missing implementation (falling back to the former lesser-arity). I am not sure if that is feasible though. Just thinking "out loud" somewhat. 😛

[alex-dixon]

This is an important feature for me as I'm working on a devtool that relies on the changes.

If this is a breaking change I'm thinking I should probably add tests that fail in order to show that and rework the implementation of the changes I've made to be non-breaking so they pass.

[rbrush]

This PR would be a breaking change as is, since the listener is a public API, and I agree we lack unit tests to expose that.

However, I doubt there are many (if any) listener implementations in the wild...we just use the listener in clara.tools.tracing for this sort of thing, which is updated appropriately here. And the protocol itself isn't yet documented, either.

So I think we can do one of:

1. Make this breaking change, declare it in the release notes, and up the version number. This isn't ideal but since I doubt there are listeners in the wild I think we can manage that.

2. Put the new extended-arity methods in a new protocol like "ITransientListenerExtended" and check have the DelegatingListener check and call that if the given listener implements this. This avoids the breaking change but adds some cruft to the code.

I'm fine with either one of these...although I'd lean towards option 1 since I doubt this will impact anyone, it's easy to adjust if we do, and it keeps the code cleaner.

[mrrodriguez]

@rbrush I agree with your points.

What I was trying to explain here before, but I don't think is better than either of your 2 points ideas at this point, is that all the actual calls through to the listener via the Clara engine seem to go through the clara.rules.listener.DelegatingListener.

This is a layer of indirection that could potentially allow for us to extend the protocol without breaking - if we defaulted to calling old arities when the new arity was "unsupported" on the underlying listener impl.

The difficulty in this approach is that there isn't a real elegant way to know which protocol functions are not implemented by a given listener impl. There are some hacky approaches I could think of, (try-catch, fallback, probably cache to "remember" after one failure) but I think it'd be better to have something like a new type express the new functions if we were trying to do this in a non-breaking way.

It seems to me that Clojure protocols freely allow you to extend them without breaking implementations The issue is that older implementations then become only partially complete. I'm not sure what sort of idioms exist for handling that situation is really.

[rbrush]

@mrrodriguez Yep, makes sense.

I guess I'll register a +1 to do this as a breaking change since I think the risks are minimal, but I'm open to going the other way if anyone has information that I don't.

[alex-dixon]

@mrrodriguez I updated the changelog to reflect a breaking change. I'm motivated to go that route. I think we have a lot more high value features we could focus our time and energy on discussing and implementing (clojure.spec, Cursive support, modify, stream processing with Onyx and Kafka, webworker support, Jess-inspired operations on lists of facts, are some I'd like to work on and discuss at some point). The consequences of not going all the way with this one seem small. Amending the changes to avoid breakage might actually backfire, resulting in code that's more difficult to maintain or less performant for 99 percent of use cases. I'd sooner do the work you suggest if we get feedback from users who say they want it. If and when that happens you've given a few solid plans here we can follow to implement it.

[mrrodriguez]

@alex-dixon Yes, I agree with both you and Ryan on this.
It seems fine to me to just make the change and be sure to note it in the changelog this time around.

[alex-dixon]

Ok. I added this to the change log : c68f95d#diff-4ac32a78649ca5bdd8e0ba38b7006a1eR4

Let me know if there’s anything else I need to do.

[WilliamParker]

+1 to just making this as a breaking change. The uplift is trivially easy (i.e. just adding unused arguments to any listener implementations) and I don't think trying to make it passive would be worth the complication in the code.

As @mrrodriguez alluded to, I'm traveling right now and don't have time to fully consider this at the moment (I'll be back after New Year's) but have we considered whether these additions are something that won't change? What sort of use(s) are you making of the new information in the listeners @alex-dixon ? In general listeners are heavily exposed to internal changes in Clara's rule networks, so depending on what you're doing perhaps an enhancement to the inspect namespace would be more stable, particularly now that it supports ClojureScript. That said, listeners exist since they are useful in some cases, we have a custom listener that we use ourselves.

[alex-dixon]

@WilliamParker I'm trying to get a stream of every insert-logical, insert-unconditional, retract, and retract-logical, and the rule that was involved if there was one in each case. I'm making a devtool for precept, so I'm sending each event with the rule firing number for the session (state number), an event number (reset each rule firing), matches, bindings, lhs, rhs, facts involved in the event ...basically everything Clara provides to the listener methods. If there's no rule involved I'm able to infer it was an "action" event (an insert from outside the session/context of a rule), or whether the event was caused by precept's schema-based truth maintenance.

Here's a couple screenshots I shared with .@mrrodriguez earlier this morning. Still a work in progress but hopefully gives some idea what I'm after:

If you're interested some of the precept changes that use the updated listeners are here CoNarrative/precept@770a644#diff-ede996a3188ef35858cf700121563f01R196.

I looked through the inspect namespace and it's not clear to me how to get information about every insert/retract operation performed (regardless of whether a rule is involved or not) in the order they occur. By using listeners (one of my favorite Clara namespaces) I'm able to easily get a stream of event data and connect it to a socket.

[WilliamParker]

These changes look good to me @alex-dixon. Your description of why you need to use a listener makes sense - session inspection is specifically intended to show the logical state of the session after the rules are finished firing, so if you need the intermediate states of the session then a listener is needed. Do keep in mind that with truth maintenance, the API is only concerned with final session states and the path Clara takes to get there can change (and possibly have intermediate states that don't really make logical sense but aren't ever returned to the user). There are reasons one might want to see the intermediate states though, among them performance profiling.

I logged #368 and #369 for some things I noticed while looking at this, but I don't think either should block moving forward on this PR.

Does anyone else want to look at this before merging?

[WilliamParker]

This looks good to me. Nobody has responded in a while and this is a low-risk change so I've merged it.