Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "surrrouning"


# 0.19.1
* Added a new field to the clara.rules.engine/Accumulator record. This could be a breaking change for any user durability implementations with low-level performance optimizations. See [PR 410](https://github.com/cerner/clara-rules/pull/410) for details.
* Performance improvements for :exists conditions. See [issue 298](https://github.com/cerner/clara-rules/issues/298).
Expand Down
6 changes: 4 additions & 2 deletions src/main/clojure/clara/macros.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(mapv (comp symbol name) (:bindings beta-node)))]
;; using the :bindings defined on the beta-node rather than `all-bindings`, as all-bindings
;; does not account for bindings that reside within expression join nodes
(com/compile-action (:bindings beta-node)
;; Using private function for now as a workaround.
(if (:ns-name production)
(if (com/compiling-cljs?)
Expand Down
54 changes: 36 additions & 18 deletions src/main/clojure/clara/rules/compiler.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
[clojure.core.reducers :as r]
[clojure.reflect :as reflect]
[clojure.set :as s]
[clojure.set :as set]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tried to clean up this binding

[clojure.string :as string]
[clojure.walk :as walk]
[schema.core :as sc]
Expand Down Expand Up @@ -803,17 +802,17 @@

;; Unsatisfied conditions remain, so find ones we can satisfy.
(let [satisfied? (fn [classified-condition]
(clojure.set/subset? (:unbound classified-condition)
bound-variables))
(s/subset? (:unbound classified-condition)
bound-variables))

;; Find non-accumulator conditions that are satisfied. We defer
;; accumulators until later in the rete network because they
;; may fire a default value if all needed bindings earlier
;; in the network are satisfied.
satisfied-non-accum? (fn [classified-condition]
(and (not (:is-accumulator classified-condition))
(clojure.set/subset? (:unbound classified-condition)
bound-variables)))
(s/subset? (:unbound classified-condition)
bound-variables)))

has-satisfied-non-accum (some satisfied-non-accum? remaining-conditions)

Expand All @@ -825,16 +824,16 @@
(remove satisfied-non-accum? remaining-conditions)
(remove satisfied? remaining-conditions))

updated-bindings (apply clojure.set/union bound-variables
updated-bindings (apply s/union bound-variables
(map :bound newly-satisfied))]

;; If no existing variables can be satisfied then the production is invalid.
(when (empty? newly-satisfied)

;; Get the subset of variables that cannot be satisfied.
(let [unsatisfiable (clojure.set/difference
(apply clojure.set/union (map :unbound still-unsatisfied))
bound-variables)]
(let [unsatisfiable (s/difference
(apply s/union (map :unbound still-unsatisfied))
bound-variables)]
(throw (ex-info (str "Using variable that is not previously bound. This can happen "
"when an expression uses a previously unbound variable, "
"or if a variable is referenced in a nested part of a parent "
Expand All @@ -855,11 +854,30 @@
(defn- non-equality-unifications
"Returns a set of unifications that do not use equality-based checks."
[constraints]
(let [[bound-variables unbound-variables] (classify-variables constraints)]
(into #{}
(for [constraint constraints
:when (non-equality-unification? constraint bound-variables)]
constraint))))
(let [[bound-variables _] (classify-variables constraints)]
(loop [[cur & more] constraints
constraints #{}
bound-variables bound-variables]
(if cur
(let [non-equity? (non-equality-unification? cur bound-variables)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you meant non-equality? here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


;; In the event that the unification is also a binding, then we will need to remove
;; 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?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(seq cur)
(equality-expression? cur))
(s/difference bound-variables
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(into #{}
(filter is-variable?)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)}

cur))
bound-variables)
constraints (if non-equity?
(conj constraints cur)
constraints)]
(recur more constraints bound-variables))
constraints))))

(sc/defn condition-to-node :- schema/ConditionNode
"Converts a condition to a node structure."
Expand Down Expand Up @@ -917,7 +935,7 @@
constraint-bindings)

new-bindings (s/difference (variables-as-keywords (:constraints condition))
parent-bindings)
parent-bindings)

join-filter-bindings (if join-filter-expressions
(variables-as-keywords join-filter-expressions)
Expand Down Expand Up @@ -1018,7 +1036,7 @@
;; have the necessary bindings.
;; See https://github.com/cerner/clara-rules/issues/304 for more details
;; and a case that behaves incorrectly without this check.
ancestor-bindings-in-negation-expr (set/intersection
ancestor-bindings-in-negation-expr (s/intersection
(variables-as-keywords negation-expr)
ancestor-bindings)

Expand Down Expand Up @@ -1224,8 +1242,8 @@
;; for use in descendent nodes.
{:beta-graph (:beta-graph new-result)
:new-ids (into (:new-ids previous-result) (:new-ids new-result))
:bindings (set/union (:bindings previous-result)
(:bindings new-result))}))
:bindings (s/union (:bindings previous-result)
(:bindings new-result))}))

;; Initial reduce value, combining previous graph, parent ids, and ancestor variable bindings.
{:beta-graph beta-with-negations
Expand Down
1 change: 1 addition & 0 deletions src/main/clojure/clara/rules/testfacts.cljc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
;; place them here as leiningen won't AOT compile test resources.
(defrecord Temperature [temperature location])
(defrecord WindSpeed [windspeed location])
(defrecord WindChill [temperature location wind-chill])
(defrecord Cold [temperature])
(defrecord Hot [temperature])
(defrecord ColdAndWindy [temperature windspeed])
Expand Down
71 changes: 70 additions & 1 deletion src/test/common/clara/test_bindings.cljc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,15 @@
query]]

[clara.rules.testfacts :refer [->Temperature ->Cold ->WindSpeed
->ColdAndWindy]]
->ColdAndWindy ->WindChill]]
[clojure.test :refer [is deftest run-tests testing use-fixtures]]
[clara.rules.accumulators :as acc]
[schema.test :as st])
(:import [clara.rules.testfacts
Temperature
Cold
WindSpeed
WindChill
ColdAndWindy]))

:cljs
Expand All @@ -33,6 +34,7 @@
[clara.rules.testfacts :refer [->Temperature Temperature
->Cold Cold
->WindSpeed WindSpeed
->WindChill WindChill
->ColdAndWindy ColdAndWindy]]
[clara.rules.accumulators :as acc]
[cljs.test]
Expand Down Expand Up @@ -369,3 +371,70 @@
fire-rules
(query cold-query)))
"The query results should not be empty for a matching location"))

;; https://github.com/cerner/clara-rules/issues/426
;; A join binding used in the same condition but different constraint will fail to compile.
(defn calc-wind-chill
[temp wind-speed]
(+ 35.74
(- (* 0.6215 temp)
(* 35.75
(Math/pow wind-speed 0.16)))
(* 0.4275 temp (Math/pow wind-speed 0.16))))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


(def-rules-test test-join-binding-followed-by-usage
{:rules [cold-windy-rule [[[Temperature
(= ?t temperature)
(= ?loc location)]
[WindSpeed
(= location ?loc)
(= ?wc (calc-wind-chill ?t windspeed))
(<= ?wc 32)]]
(insert! (->WindChill ?t ?loc ?wc))]]
:queries [wind-chill-query [[]
[[WindChill
(= ?t temperature)
(= ?wind-chill wind-chill)
(= ?loc location)]]]]
:sessions [empty-session [cold-windy-rule wind-chill-query] {}]}
;; The overall intent of this test is to prove that the rules above compile and
;; operate as expected.
(is (= [{:?loc "LHR"
:?t 36
:?wind-chill (calc-wind-chill 36 7)}]
(-> empty-session
(insert (->Temperature 36 "LHR"))
(insert (->WindSpeed 7 "LHR"))
fire-rules
(query wind-chill-query)))
;; Not the intent of the test, but a distinct message in the event the test fails.
"The query should return the wind-chill for the given location since the wind chill is under 32 degrees"))

;; https://github.com/cerner/clara-rules/issues/426
;; Due to the way we compile the beta-network for cljs this test would fail for cljs because the binding
;; expression join node(?wc) would not be recognised correctly in the production node. Thus ?wc would be
;; assumed to be a variable defined in this ns and would resolve to nil at runtime.
(def-rules-test test-join-binding-used-in-a-produciton-node
{:rules [cold-windy-rule [[[Temperature
(= ?t temperature)
(= ?loc location)]
[WindSpeed
(= location ?loc)
(= ?wc (calc-wind-chill ?t windspeed))]]
(insert! (->WindChill ?t ?loc ?wc))]]
:queries [wind-chill-query [[]
[[WindChill
(= ?t temperature)
(= ?wind-chill wind-chill)
(= ?loc location)]]]]
:sessions [empty-session [cold-windy-rule wind-chill-query] {}]}
(is (= [{:?loc "LHR"
:?t 36
:?wind-chill (calc-wind-chill 36 7)}]
(-> empty-session
(insert (->Temperature 36 "LHR"))
(insert (->WindSpeed 7 "LHR"))
fire-rules
(query wind-chill-query)))
;; Not the intent of the test, but a distinct message in the event the test fails.
"The query should return the wind-chill for the given location since there is a Temperature and WindSpeed"))