(#426) Join binding followed by usage of binding leads to unresolvabl… #427

EthanEChristian · 2019-04-16T18:06:03Z

…e symbol at compilation

…to unresolvable symbol at compilation

EthanEChristian · 2019-04-16T18:07:33Z

src/main/clojure/clara/rules/compiler.clj

@@ -9,7 +9,6 @@
            [clojure.core.reducers :as r]
            [clojure.reflect :as reflect]
            [clojure.set :as s]
-            [clojure.set :as set]


tried to clean up this binding

EthanEChristian · 2019-04-19T19:42:43Z

src/main/clojure/clara/macros.clj

@@ -174,8 +174,10 @@
                                                     ;; that a rule wouldn't, or at least shouldn't for
                                                     ;; clarity, start the names of other locals or vars
                                                     ;; with "?".
-                                                     (mapv (comp symbol name) all-bindings))]
-              (com/compile-action all-bindings


Looking at this again, i don't think all-bindings is required at all after these changes.

If I understand this correctly all-bindings is being populated from the constraints after this transformation and that is why this change is needed?

mrrodriguez

I think there may be issues to address still.

mrrodriguez · 2019-04-17T17:50:17Z

CHANGELOG.md

@@ -1,5 +1,8 @@
 This is a history of changes to clara-rules.

+# 0.20.0-SNAPSHOT
+* Correct implementation surrrouning bindings in ExpressionJoinNodes. See [Issue 426](https://github.com/cerner/clara-rules/issues/426)


Typo: "surrrouning"

mrrodriguez · 2019-04-22T15:42:26Z

src/main/clojure/clara/rules/compiler.clj

+              bound-variables (if (and non-equity?
+                                       (seq cur)
+                                       (equality-expression? cur))
+                                (s/difference bound-variables


Perhaps it'd be faster to use disj here.

(->> cur (filter is-variable?) (apply disj bound-variables))

Edit: this approach may need to be changed and this particular code-change may no longer be relevant, see above.

mrrodriguez · 2019-04-22T15:44:38Z

src/main/clojure/clara/rules/compiler.clj

+           constraints #{}
+           bound-variables bound-variables]
+      (if cur
+        (let [non-equity? (non-equality-unification? cur bound-variables)


I think you meant non-equality? here?

mrrodriguez · 2019-04-22T16:06:34Z

src/main/clojure/clara/rules/compiler.clj

+           constraints #{}
+           bound-variables bound-variables]
+      (if cur
+        (let [non-equity? (non-equality-unification? cur bound-variables)


After exploring the non-equality-unification? fn again, I noticed the doc string is in the wrong position - so it isn't a doc string. Perhaps can clean that up in this PR.

https://github.com/cerner/clara-rules/blob/65869b0037e9677be9404d1750a20156bbf5b616/src/main/clojure/clara/rules/compiler.clj#L597

The doc string is coming after arg list.

mrrodriguez · 2019-04-22T17:04:29Z

src/main/clojure/clara/rules/compiler.clj

+              ;; the bound variable from the previously bound variables. This prevents further constraints
+              ;; that use this variable from being considered "equality-unifications".
+              ;; See https://github.com/cerner/clara-rules/issues/426 for more info.
+              bound-variables (if (and non-equity?


I struggled with this a bit. I will summarize my understanding for clarity.

The issue #426 description had an example with this form of

[A (= ?a a)] [B (= ?r (conj b ?a)) (not-empty? ?r)]

Now consider the constraints for B fact condition:

(= ?r (conj b ?a)) (not-empty? ?r)

What is happening is that the (= ?r (conj b ?a)) is true for (non-equality-unification? cur bound-variables) because it has the sub-expression (conj b ?a) which is unifying on an unknown to this condition variable, in a way that is not using = (which has a simplified/optimized/hash-based impl).

I think it is important to note that this particular fn , non-equality-unifications is used to find all constraints that will be separated away from the "simple" expressions and moved into a "join expression" that deals with the non-equality unification.

Now (classify-variables constraints) is classifying this new variable binding ?r as a "bound variable" in bound-variables. However, it isn't necessarily "previously bound" if constraints are moved around during compilation. Subsequent constraints that are not moved with the non-equality unifications, could be broken, since the classification isn't accurate anymore if they are not also moved.

Concretely, the next constraint is (not-empty? ?r) and ?r is classified as previously bound now.
This means that:

(non-equality-unification? '(not-empty? ?r) #{?r}) ;;= false

This would result in (not-empty? ?r) being "skipped" in this fn, and consequently not "moved" with the rest of the "join filter" expression constraints later.

This approach works around this, by doing what Ethan appropriately called "re-classification". When a non-equality unification is detected, it has to remove it's newly introduced bindings from the classification as a mechanism to make other dependent constraints move along with it.

I can follow this approach. However, it does seem like a sneaky way to utilize how bound variables are used to detect non-equality unifications.
The reason I find it "sneaky" is that the logic it is doing is somewhat assuming that the caller is planning to move constraints around. It has the semantics that if it returns a constraint, it needs to also return all the constraints that may depend on that constraint's new bindings.

However, it seems that this may not work for some cases still:

(defrecord A [a]) (defrecord B [b]) (defrule a-rule2 [A (= ?a a)] [B (= ?r (conj b ?a)) (= ?r #{:a :b})] => (println "doesn't matter"))

ie. the 2nd constraint is an equality binding on ?r.

This compiles, but it isn't producing a semantically correct network.

The equality unification on ?r in (= ?r #{:a :b}) isn't being considered dependent on the ?r introduced by (= ?r (conj b ?a)) that is going to be moved into the join filter later.
You can see this by running above with:

(-> (mk-session [a-rule2] :cache false) (insert (->A :c) (->B #{:b})) fire-rules) ;; Prints "doesn't matter"

I think the solution to this is in not trying to trying to manipulate only the classified bound-variables to find the dependent constraints. A more "brute-force" search may be needed. In an interation where this condition is detected of the form of (= ?r (conj b ?a)), I think all subsequently dependent constraints that refer to ?r need to be added ot the constraints immediately and removed from more - in terms of the locals used in the changes to the non-equality-unifications fn.

Regarding the a-rule2 case you describe above @mrrodriguez it looks to me like the problem is that the alpha node is creating the ?r binding, but the join filter is then rebinding it rather than testing for equality with the value created in the alpha network.

[5 :alpha-expr] [#object[clara.test_rules$eval17454$fn__17457 0x2fe5411 "clara.test_rules$eval17454$fn__17457@2fe5411"] {:file "*cider-repl localhost*", :compile-ctx {:condition {:type clara.test_rules.B, :constraints [(= ?r #{:b :a})], :original-constraints [(= ?r (conj b ?a)) (= ?r #{:b :a})]}, :env nil, :msg "compiling alpha node"}, :alpha-expr (clojure.core/fn [?__fact__ ?__env__] (clojure.core/let [this ?__fact__ ?__bindings__ (clojure.core/atom {})] (clojure.core/let [?r #{:b :a}] (clojure.core/swap! ?__bindings__ clojure.core/assoc :?r ?r) @?__bindings__)))}], [2 :join-filter-expr] [2 :join-filter-expr] [#object[clara.test_rules$eval17454$fn__17459 0x60678a61 "clara.test_rules$eval17454$fn__17459@60678a61"] {:compile-ctx {:condition {:type clara.test_rules.B, :constraints [(= ?r #{:b :a})], :original-constraints [(= ?r (conj b ?a)) (= ?r #{:b :a})]}, :join-filter-expressions {:type clara.test_rules.B, :constraints [(= ?r (conj b ?a))]}, :env nil, :msg "compiling expression join node"}, :join-filter-expr (clojure.core/fn [?__token__ ?__fact__ ?__element-bindings__ ?__env__] (clojure.core/let [this ?__fact__ b (.-b ?__fact__) ?a (clojure.core/-> ?__token__ :bindings :?a) ?r (get ?__element-bindings__ :?r) ?__bindings__ (clojure.core/atom {})] (clojure.core/let [?r (conj b ?a)] (clojure.core/swap! ?__bindings__ clojure.core/assoc :?r ?r) @?__bindings__)))}]

compile-constraints takes a collection of variables that it should not rebind, but in compile-join-filter this only includes variables from the ancestors. At first glance the solution would seem to be to pass along the element-bindings passed to compile-join-filter to compile-constraints. However, on closer examination, it doesn't appear to me that the element-bindings are necessarily correct here. They are populated from the :new-bindings of the beta network, which are just the bindings in the condition that aren't from the ancestors as can be seen in their population here.

In order to fix this problem I think we need to store (correct) information on what bindings are created in the alpha network and which are created in the beta network, rather than just new-bindings information.

Regarding this immediate PR, my current thinking is that there shouldn't be any need to start from the viewpoint of all bindings which are created in the condition; we can start from a "blank slate" of no bindings and proceed from there. We could then generate a list of bindings created in the alpha network and of bindings created in the beta network and use this to resolve this issue and the one described by @mrrodriguez above. Intuitively, the current solution seems a bit like we have incorrect binding analysis and are "cleaning it up" after the fact, while I'd suggest the cleaner solution would be to correct the binding analysis upfront. Note also that we do have the list of bindings from the ancestors available should it be needed, although at the moment I don't think we do. I also haven't tried implementing this concretely so it's possible that I'm missing something.

This may also bring up issues of whether we want the constraints to be order-independent as well and figuring out what the current behaviour is.

WilliamParker

Good find, I think there are some issues to address before merging though. It might also be most expedient to fix the additional issue @mrrodriguez pointed out if the fixes are related/to the same areas of the code even if they aren't the same issue.

WilliamParker · 2019-05-12T22:43:28Z

src/main/clojure/clara/rules/compiler.clj

+                                       (equality-expression? cur))
+                                (s/difference bound-variables
+                                              (into #{}
+                                                    (filter is-variable?)


This is something of an edge case, but this can result in some constraints that are actually resolvable in the alpha network being considered non-equality-unifications since the variable could be previously bound.

Example:

(defrule myrule [Temperature (= ?t temperature) (= ?loc location)] [WindSpeed (= location ?loc) (= ?windspeed windspeed) (= ?wc (calc-wind-chill ?t ?windspeed) ?loc) (windy-for-location? ?loc ?windspeed)] => (println "Doesn't matter")) (-> myrule :lhs second :constraints non-equality-unifications) #{(= ?wc (calc-wind-chill ?t ?windspeed) ?loc) (windy-for-location? ?loc ?windspeed)}

WilliamParker · 2019-05-12T23:10:25Z

src/main/clojure/clara/rules/compiler.clj

+              ;; the bound variable from the previously bound variables. This prevents further constraints
+              ;; that use this variable from being considered "equality-unifications".
+              ;; See https://github.com/cerner/clara-rules/issues/426 for more info.
+              bound-variables (if (and non-equity?


Regarding the a-rule2 case you describe above @mrrodriguez it looks to me like the problem is that the alpha node is creating the ?r binding, but the join filter is then rebinding it rather than testing for equality with the value created in the alpha network.

[5 :alpha-expr] [#object[clara.test_rules$eval17454$fn__17457 0x2fe5411 "clara.test_rules$eval17454$fn__17457@2fe5411"] {:file "*cider-repl localhost*", :compile-ctx {:condition {:type clara.test_rules.B, :constraints [(= ?r #{:b :a})], :original-constraints [(= ?r (conj b ?a)) (= ?r #{:b :a})]}, :env nil, :msg "compiling alpha node"}, :alpha-expr (clojure.core/fn [?__fact__ ?__env__] (clojure.core/let [this ?__fact__ ?__bindings__ (clojure.core/atom {})] (clojure.core/let [?r #{:b :a}] (clojure.core/swap! ?__bindings__ clojure.core/assoc :?r ?r) @?__bindings__)))}], [2 :join-filter-expr] [2 :join-filter-expr] [#object[clara.test_rules$eval17454$fn__17459 0x60678a61 "clara.test_rules$eval17454$fn__17459@60678a61"] {:compile-ctx {:condition {:type clara.test_rules.B, :constraints [(= ?r #{:b :a})], :original-constraints [(= ?r (conj b ?a)) (= ?r #{:b :a})]}, :join-filter-expressions {:type clara.test_rules.B, :constraints [(= ?r (conj b ?a))]}, :env nil, :msg "compiling expression join node"}, :join-filter-expr (clojure.core/fn [?__token__ ?__fact__ ?__element-bindings__ ?__env__] (clojure.core/let [this ?__fact__ b (.-b ?__fact__) ?a (clojure.core/-> ?__token__ :bindings :?a) ?r (get ?__element-bindings__ :?r) ?__bindings__ (clojure.core/atom {})] (clojure.core/let [?r (conj b ?a)] (clojure.core/swap! ?__bindings__ clojure.core/assoc :?r ?r) @?__bindings__)))}]

compile-constraints takes a collection of variables that it should not rebind, but in compile-join-filter this only includes variables from the ancestors. At first glance the solution would seem to be to pass along the element-bindings passed to compile-join-filter to compile-constraints. However, on closer examination, it doesn't appear to me that the element-bindings are necessarily correct here. They are populated from the :new-bindings of the beta network, which are just the bindings in the condition that aren't from the ancestors as can be seen in their population here.

In order to fix this problem I think we need to store (correct) information on what bindings are created in the alpha network and which are created in the beta network, rather than just new-bindings information.

Regarding this immediate PR, my current thinking is that there shouldn't be any need to start from the viewpoint of all bindings which are created in the condition; we can start from a "blank slate" of no bindings and proceed from there. We could then generate a list of bindings created in the alpha network and of bindings created in the beta network and use this to resolve this issue and the one described by @mrrodriguez above. Intuitively, the current solution seems a bit like we have incorrect binding analysis and are "cleaning it up" after the fact, while I'd suggest the cleaner solution would be to correct the binding analysis upfront. Note also that we do have the list of bindings from the ancestors available should it be needed, although at the moment I don't think we do. I also haven't tried implementing this concretely so it's possible that I'm missing something.

This may also bring up issues of whether we want the constraints to be order-independent as well and figuring out what the current behaviour is.

WilliamParker · 2019-05-12T23:38:39Z

src/test/common/clara/test_bindings.cljc

+     (- (* 0.6215 temp)
+        (* 35.75
+           (Math/pow wind-speed 0.16)))
+     (* 0.4275 temp (Math/pow wind-speed 0.16))))


I'm guessing this is a formula for wind chill? It seems unnecessarily complicated to use a real formula here since what we're testing has nothing to with this. Also does "Math/pow" work in ClojureScript?

WilliamParker · 2019-05-12T23:39:10Z

src/main/clojure/clara/rules/compiler.clj

+           constraints #{}
+           bound-variables bound-variables]
+      (if cur
+        (let [non-equity? (non-equality-unification? cur bound-variables)


WilliamParker · 2019-05-12T23:50:38Z

src/main/clojure/clara/macros.clj

@@ -174,8 +174,10 @@
                                                     ;; that a rule wouldn't, or at least shouldn't for
                                                     ;; clarity, start the names of other locals or vars
                                                     ;; with "?".
-                                                     (mapv (comp symbol name) all-bindings))]
-              (com/compile-action all-bindings


If I understand this correctly all-bindings is being populated from the constraints after this transformation and that is why this change is needed?

(oracle-samples#426) Join binding followed by usage of binding leads …

65869b0

…to unresolvable symbol at compilation

EthanEChristian mentioned this pull request Apr 16, 2019

Join binding followed by usage of binding leads to unresolvable symbol at compilation #426

Open

EthanEChristian commented Apr 16, 2019

View reviewed changes

EthanEChristian commented Apr 19, 2019

View reviewed changes

mrrodriguez requested changes Apr 22, 2019

View reviewed changes

WilliamParker requested changes May 12, 2019

View reviewed changes

EthanEChristian changed the base branch from master to main July 9, 2020 15:19

EthanEChristian marked this pull request as draft July 9, 2020 15:19

EthanEChristian self-assigned this Jul 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(#426) Join binding followed by usage of binding leads to unresolvabl… #427

(#426) Join binding followed by usage of binding leads to unresolvabl… #427

EthanEChristian commented Apr 16, 2019

EthanEChristian Apr 16, 2019

EthanEChristian Apr 19, 2019

WilliamParker May 12, 2019

mrrodriguez left a comment

mrrodriguez Apr 17, 2019

mrrodriguez Apr 22, 2019

mrrodriguez Apr 22, 2019

WilliamParker May 12, 2019

mrrodriguez Apr 22, 2019

mrrodriguez Apr 22, 2019

WilliamParker May 12, 2019

WilliamParker left a comment

WilliamParker May 12, 2019

WilliamParker May 12, 2019

WilliamParker May 12, 2019

WilliamParker May 12, 2019

WilliamParker May 12, 2019

(#426) Join binding followed by usage of binding leads to unresolvabl… #427

Are you sure you want to change the base?

(#426) Join binding followed by usage of binding leads to unresolvabl… #427

Conversation

EthanEChristian commented Apr 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrrodriguez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilliamParker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment