Provide better feedback when constraints throw exceptions
Closed this issue · 10 comments
It is currently difficult to identify which constraint threw an exception during alpha node activation and retraction and during the activation and retraction of expression join nodes. This is important because even the most innocuous constraint can throw an exception. For example, the constraint (> ?rate 1) throws in the following code because rate is nil in the provided fact.
(ns test
(:require [clara.rules :as rules]
[clara.rules.dsl :as dsl]))
(defrecord RateToUSD [currency rate])
(let [rule-template (dsl/parse-rule [[RateToUSD (= ?rate rate)
(= ?currency currency)
(> ?rate 1)]]
(println "Ok"))
rule (assoc rule-template :name "find-all-rates")]
(-> (rules/mk-session [rule] :cache false)
(rules/insert (->RateToUSD "CAD" nil))
(rules/fire-rules)))
A NullPointerException is thrown by clojure.lang.Numbers.ops with the following stack trace.
NullPointerException
clojure.lang.Numbers.ops (Numbers.java:1013)
clojure.lang.Numbers.gt (Numbers.java:229)
clojure.lang.Numbers.gt (Numbers.java:3864)
test/eval24511/fn--24512/fn--24514 (form-init2109496090701568388.clj:2)
test/eval24511/fn--24512 (form-init2109496090701568388.clj:2)
clara.rules.engine/alpha-node-matches/iter--21928--21932/fn--21933 (engine.cljc:440)
clojure.lang.LazySeq.sval (LazySeq.java:40)
clojure.lang.LazySeq.seq (LazySeq.java:49)
clojure.lang.RT.seq (RT.java:507)
clojure.core/seq--4128 (core.clj:137)
clara.rules.engine.AlphaNode/iter--21960--21964/fn--21965 (engine.cljc:459)
clojure.lang.LazySeq.sval (LazySeq.java:40)
Notice how no information is provided as to which constraint threw the exception, which fact was being processed, or what the bindings were when the constraint threw the exception. This makes it difficult to debug constraints that throw exceptions. Better exception messages should help the development process. For example, printing the following message could help in addition to the stack trace.
Exception raised during execution of constraint
(> ?rate 1)
of condition
[test.RateToUSD (= ?rate rate) (= ?currency currency) (> ?rate 1)]
of rule
find-all-rates
when processing fact
#test.RateToUSD{:currency "CAD", :rate nil}
with bindings
{:?rate nil, :?currency "CAD"}
A similar message could also be printed for a throwing filter function in an expression join node like the one below.
(let [rule-template (dsl/parse-rule [[RateToUSD (= ?to-rate rate)
(= ?to-currency currency)]
[RateFromUSD (= ?from-currency currency)
(> (* rate ?to-rate) 1)]]
(println "Ok"))
rule (assoc rule-template :name "find-currency-pairs")]
(-> (rules/mk-session [rule] :cache false)
(rules/insert (->RateToUSD "CAD" nil))
(rules/insert (->RateFromUSD "EUR" 0.95))
(rules/fire-rules)))Exception raised during execution of constraint
(> (* rate ?to-rate) 1)
of condition
[test.RateFromUSD (= ?from-currency currency) (> (* rate ?to-rate) 1)]
of rule
find-currency-pairs
when processing fact
#test.RateFromUSD{:currency "EUR", :rate 0.95}
with bindings
{:?to-rate nil, :?to-currency "CAD"}
And another when we switch the fact with a nil rate.
(let [rule-template (dsl/parse-rule [[RateToUSD (= ?to-rate rate)
(= ?to-currency currency)]
[RateFromUSD (= ?from-currency currency)
(> (* rate ?to-rate) 1)]]
(println "Ok"))
rule (assoc rule-template :name "find-currency-pairs")]
(-> (rules/mk-session [rule] :cache false)
(rules/insert (->RateToUSD "CAD" 1.33))
(rules/insert (->RateFromUSD "EUR" nil))
(rules/fire-rules)))Exception raised during execution of constraint
(> (* rate ?to-rate) 1)
of condition
[test.RateFromUSD (= ?from-currency currency) (> (* rate ?to-rate) 1)]
of rule
find-currency-pairs
when processing fact
#test.RateFromUSD{:currency "EUR", :rate nil}
with bindings
{:?to-rate 1.33, :?to-currency "CAD"}
I haven't shown it here, but nodes can correspond to multiple rules, so the of rule section must list all related rules.
I have prepared a pull request with an example implementation of these messages, but I'm not sure whether the approach is ideal because I'm new to Clara, to Clojure, and to pull requests in general.
I love the idea! I'll take a look at the code review in the next day or so and comment there, but I'm sure we can get some variation of this in.
Wonderful, thank you for having a look.
+1 to this sort of error message improvement from me as well; I haven't looked at the PR in detail yet but I like the idea.
@carlosphillips there are some issues to address on the PR, but overall this is good work and a very nice enhancement.
Sounds great. I can make some changes if that would help. Basically, I'm happy to follow your workflow. One of the things that I wonder about is what information should be captured in the compiled code versus in node fields. For example, where should the constraint expressions in quoted form be captured?
"For example, where should the constraint expressions in quoted form be captured?" => are you talking about the :constraint-exp key in the error map returned from the constraint? Did you have another proposal for where to get that from?
In general, I see some places here where you're deriving data at runtime after an exception is thrown that could be calculated at rulebase compilation time - for example the names of rules downstream from a node - but since this work is only performed when an exception is thrown I don't see the performance impact of that as significant. I'd prioritize ease of your development and maintainability when deciding where to get information when the work will only be done when an exception is thrown.
Thank you
As discussed at #256 (comment) I didn't merge #256 but rather applied the following commits directly to master:
which in effect merged the PR with some modifications to only wrap a try-catch around entire conditions, rather than individual constraints, since the latter proved to have performance problems. Thus, if you have a condition like
[ColdAndWindy (< temperature 0) (> windspeed 50)]
and the temperature is nil, the exception thrown will show the entire condition in the exception thrown rather than the individual constraint
(< temperature 0)
I have logged #265 to extend similar error handling to accumulators. At this point I don't see anything left to do on this issue and will close it on Monday unless there is disagreement on doing so in which case we can discuss it. Thanks again @carlosphillips.
Closing as discussed above.