Fork me on GitHub
#clara
<
2018-05-21
>
dominicm05:05:42

That equality is crazy! Thanks!

dominicm05:05:14

http://clojuredocs.org/clojure.core/defrecord > and will defined Java .hashCode and .equals consistent with the contract for java.util.Map. Looks like this is intentional

dominicm07:05:29

(definterface Foo
  (getB []))

(deftype C [a]
  Foo
  (getB [this] a))

(deftype B [a]
  Foo
  (getB [this] a))
Onto java beans I go!

mikerod10:05:06

Clara just uses clojure =. Not sure the issue above? Clojure = takes type into consideration with records.

dominicm10:05:25

@mikerod The code that @thegeez referred to uses a java.util.HashMap, which uses java's notion of equality.

dominicm10:05:54

Fortunately for me, I am in an intersection of 2 points: 1) My data is all the same keys 2) I use macros for generating my types So I've been able to switch to beans with deftype rather trivially, and that's resolved everything.

mikerod11:05:22

@dominicm that’s interesting. I don’t know how you were encountering the use of platform/group-by-seq though. It does sound to me like there could be potential edge cases around platform/group-by-seq though on the jvm-impl since it does make use of Java collections to (a) preserve consistent order (b) do the grouping and order preservation efficiently.

thegeez12:05:25

@mikerod if the promise is that clojure = is used, than @dominicm’s example shows a bug

thegeez12:05:10

(defrecord ClauseOne [b])
(defrecord ClauseTwo [b])

(defrule rule-one
  [ClauseOne (= ?b b)]
  [ClauseTwo (= ?b b)]
 =>
  (println "Match found:" ?b )
  )

(defrecord WrapAlice [num])
(defrecord WrapBob [num])

(comment
  (clear-ns-productions!)
  (let [alice (->WrapAlice 1)
        bob (->WrapBob 1)]
    (println "(= alice bob)" (= alice bob))
    (println "(.equals alice bob)" (.equals alice bob))
    (-> (mk-session)
        (insert
         (->ClauseOne alice)
         ;; Line below is important, without it the wrong behavior does not surface
         (->ClauseTwo alice)
         (->ClauseTwo bob)
         )
        (fire-rules)
        ))
  ;;(= alice bob) false
  ;;(.equals alice bob) true
  ;; Match found: #clara_bug.core.WrapAlice{:num 1}
  ;; Match found: #clara_bug.core.WrapBob{:num 1} ;; <- should not happen
  )

mikerod12:05:28

@thegeez I think this actually is a bug

mikerod12:05:46

I had suspicion of it being one as soon as I saw this discussion above regarding platform/group-by-seq

mikerod12:05:45

platform/group-by-seq was added on the JVM-Clara to make the operation of grouping more efficient, as well as to have deterministic ordering of the seq it returns on each run to avoid variance in rule performance. The use of Java collections for this can cause subtle problems when the bindings of fact fields are record types that are Object.equals() from Java, but not clojure.core/=. This is the case with Clj record since their Java-interop form uses Java map equality instead of incorporating the type.

thegeez12:05:04

@mikerod this might be of use: https://github.com/ztellman/bizarro-collections to get a speedy group-by-seq with the semantics of clojure =

mikerod12:05:57

I believe the issue is fairly isolated to the platform/group-by-seq. I think the solution would be to stop using a Java-based equality hashmap impl. A clj based one would be needed instead. It may be slightly tricky to do to keep similar performance characteristics. (it’s a hot spot in the rule engine).

mikerod12:05:25

@thegeez maybe, but we need a “linked hash map”

mikerod12:05:37

the deterministic “order preservation” is an important characteristic

mikerod12:05:00

I believe the bizarro only has hash map (unordered)

mikerod12:05:39

a java.util.LinkedHashMap is used to do the grouping by operations, then it is seq’ed in platform/group-by-seq. The order the seq comes out, we want to be the same given the same input, independently from one process/machine to another (really, the Java runtime execution)

mikerod12:05:11

If we didn’t need the order preservation part, we could just use a clj transient map and likely not have a problem with performance

mikerod13:05:03

One solution would just be to write some sort of collection in Clara to fit this goal. That maybe the smoothest path forward.

thegeez13:05:56

I don't know of one of the shelve

thegeez13:05:32

But when does the case occur that the order is different between process? And when do these two different orderings meet?

mikerod13:05:56

it has quite a bit of detail on this

mikerod13:05:15

determinism from one JVM instance to another is pretty important in some production scenarios

mikerod13:05:39

platform/group-by-seq is hit in a hot spot in the engine - it has a fairly large effect on performance

mikerod13:05:06

the order of its results can play a large role in the activity that takes place within the engines evaluation of the network facts

mikerod13:05:35

If the order changes from run to run, then the performance characteristics change too. This can make it hard to track down a bottleneck that comes up only on occasion

mikerod13:05:17

Also, different orders of evaluation could potentially even lead to things like exceptions thrown in defects in written rules. However, may only sometimes these defects would manifest themselves - aka hard to reproduce problems

mikerod13:05:54

A while back, all known places within Clara’s engine evaluation and compiler were made deterministic run-over-run to alleviate these issues

thegeez13:05:18

This is quickly going beyond what I know of rules systems, which is very little. I'm only surprised to see the "order changes from run to run", I would expect the ordering to be arbitrary, but the same from run to run with the same input data

mikerod13:05:00

@thegeez the ordering guarantees are arbitrary from a user-level semantics

mikerod13:05:10

and the outputs are produced in a deterministic way from that level

mikerod13:05:21

I’m talking on more of a low-level evaluation within the engine

mikerod13:05:34

It is basically opaque to the user-level semantics

mikerod13:05:20

however, it has real world implications, (a) deterministic performance characteristics (b) deterministic order things actually are evaluated (mostly useful to reproduce error cases)

mikerod13:05:12

Things get more involved if you consider using the truth maintenance system within Clara, ie performing a typical clara.rules/insert! on a rule right-hand side (RHS).

mikerod13:05:35

The engine works to put the system into a consistent logical state

mikerod13:05:45

The “works” part, is where time is spent

mikerod13:05:16

There are different orders/paths to take to reach the same consistent logical state, depending on how rules are chosen for evaluation, how facts are propagated from node to node in the internal network etc

mikerod13:05:32

The determinism I’m talking about is in being deterministic in those inner choices taken

thegeez13:05:26

If "defsomething A, defsomething B, execute" should do the same as "defsomething B, defsomething A, execute" I would expect the usage of a HashMap instead of a LinkedHashMap, with its insertion-order preserving. That part is still fuzzy in my thinking

mikerod14:05:37

@thegeez I guess I’m not able to explain it correctly

mikerod14:05:57

Yes, they can do the same thing, however, the way they accomplish it may be different depending on the order

mikerod14:05:08

hash map can arbitrarily change the order

mikerod14:05:13

and it does

mikerod14:05:37

For example, we had correct rules running in a production case before. However, about 1 in 10 times, it performed about 100x worse

mikerod14:05:58

that 1 in 10 time, was when the hash map ordered 2 groups in a flipped way

mikerod14:05:14

the engine then went on to do extra work, it hit a performance bottleneck case

mikerod14:05:26

it still was functionally correct, it just went down a “bad path” performance-wise

mikerod14:05:53

fixing the determinism allowed the case to be pinpointed and recreated in a consistent way, the bottleneck was then fixed

mikerod14:05:45

There are many places within the engine evaluation where the “order things are done” can make measurable performance differences. Even if the outcome is always the same no matter the order.

mikerod14:05:15

It is hard to explain most of these, since they are detailed things about the engine/network/fact propagation

mikerod14:05:18

Also, truth maintenance can evaluate a rule, evaluated its RHS actions, like insert!, but then later, realize that that rule isn’t satisfied anymore due to some other rule that evaluated. The engine can “retract” the work done by the rule in that case.

mikerod14:05:37

There are cases (many) where it’d be better to never have evaluated that rule at all, it wasted time

mikerod14:05:29

The situation is really that the engine can at times “speculatively” evaluate a rule that is later found to be invalidated by other rules. There are ways for user-level directed rule orderings, and also there are engine-level “heuristics” or similar that attempt to avoid premature evaluation

mikerod14:05:46

So a rule could have a RHS that says (throw (Exception.))

mikerod14:05:06

This rule may never be true after fire-rules, however, during the truth maintenance cycle, it could actually be evaluated

mikerod14:05:22

So if you have non-determinism in that cycle, you may only sometimes hit the exception side-effect

mikerod14:05:00

It doesn’t (and likely wouldn’t) be something trivial like that. It could be something less obvious like (insert! (call-my-fn-that-has-a-defect-for-some-args ?x))

mikerod14:05:36

If that rule sometimes evaluated, but needs to retract later by the truth maintenance, and this only happens 1 in 10 times, you may have a 1 in 10 times sort of scenario you are trying to recreate to debug your defect in your rules

mikerod14:05:30

This will manifest itself in cases like, our tests fail arbitrarily at times when run on our build servers, but when we rerun the build/retest the error is gone

mikerod14:05:07

The determinism of ordering used within these areas of Clara help both of these scenarios, the performance situations, and the finding failures scenarios situation

thegeez16:05:26

@mikerod Thanks for the explanation

alex-dixon16:05:02

@mikerod ditto that was great

zylox16:05:31

we should probably codify that mentality somwhere because we do think of it as first class here when considering changes

zylox16:05:40

(here being cerner)

zylox16:05:22

not sure where though. When writing rules it shouldnt be a consideration or a concern to the user

curtis.shaffer20:05:43

engine behavior? or guiding principles perhaps?

wparker20:05:32

I think https://github.com/cerner/clara-rules/blob/master/CONTRIBUTING.md is intended for that purpose in the GitHub templates, perhaps something there?

zylox20:05:26

that makes a degree of sense to me