Clojurians Log v2

I'm seeing what I think is unexpected behavior. I've boiled it down to a simple example which I'm hoping is either a bug or can be explained such that I can understand how I'm using domino incorrectly. Here is the code:

I would expect :output-1 to be resolved prior to :output-2 being called. However, that does not seem to be the case. The result of function-1 is making its way into domino.core/db but the value passed to function-2, which clearly has a dependency on :input-1 and :output-1, is nil for :output-1.

Am I doing something incorrect in setting up this model?

I'm using version 0.2.0 which probably should be upgraded.

Just to be sure, I switched to 0.3.2 and same output.

And for completeness, here is the state of domino.core/db

{:input-1 "hello",
 :input-2 "good-bye",
 :output-1 {:input-1 "hello", :input-2 "good-bye"},
 :output-2
 {:message {:input-1 "hello", :output-1 nil}, :outputs {:output-2 nil}}}

yeah I was just trying it locally with 0.3.2 and seeing the same as well

it does look like a bug to me

oh

maybe not a bug 🙂

nope nevermind 🙂

Something must have changed because I'm pretty sure this was working in an earlier version. I'm going to try it against something prior to 0.2.0 which is what I was using.

hmm yeah I was thinking we had a test case for a similar scenario

I'm a little confused because I wasn't using any version prior to 0.2.0.

for example this test seems like it should also fail:

(deftest test-cascading-events
  (test-graph-events
    [{:inputs  [[:a]]
      :outputs [[:b] [:c]]
      :handler (fn [ctx {:keys [a]} {:keys [b c]}] {:b (+ a b) :c (+ a c)})}
     {:inputs  [[:c]]
      :outputs [[:d]]
      :handler (fn [ctx {:keys [c]} _] {:d (inc c)})}]
    [[[:a] 1] [[:b] 1]]
    {::core/db       (assoc default-db :a 1 :b 2 :c 1 :d 2)
     ::core/change-history [[[:a] 1]
                      [[:b] 1]
                      [[:b] 2]
                      [[:c] 1]
                      [[:d] 2]]}))

it looks like we set this test up to use the original value of c though

ah ok, so initial db is

(def default-db {:a 0, :b 0, :c 0, :d 0, :e 0, :f 0, :g 0 :h {:i 0}})

and we expect d to be 2 which mean c has to be set to 1

but it looks like the same cascading scenario

Is ::core/db the initial state of the db or what you expect to get? If it is what you expect to get, then the test looks like it would break as the value of :c in the first event would be nil and (+ 1 nil) (which is what (+ a c) would translate to) will throw a null pointer expection. Assuming that is not happening in the test suite, then it would seem that ::core/db is being used as the initial state? Maybe it it is being used (incorrectly) for initial state and expected output or something like that?

Or maybe test-graph-events is doing something else to the initial value of :c?

if I recall correctly, the ::core/db that the event sees should be the latest updated version

Wouldn't c be nil in the first event handler?

it uses the default-db that sets it to 0

okay. That makes sense

that is part of how test-graph-events works.

(let [schema {:model
                [[:a {:id :a}]
                 [:b {:id :b}]
                 [:c {:id :c}]
                 [:d {:id :d}]]
                :events
                [{:inputs  [:a]
                  :outputs [:b :c]
                  :handler
                           (fn [ctx {:keys [a] :as inputs}
                                {:keys [b c]
                                 :as outputs}]
                             (println "event 1"
                                      "\ninputs:" inputs
                                      "\noutputs:" outputs)
                             {:b (+ a b) :c (+ a c)})}
                 {:inputs  [:c]
                  :outputs [:d]
                  :handler
                           (fn [ctx {:keys [c] :as inputs} outputs]
                             (println "event 2"
                                      "\ninputs:" inputs
                                      "\noutputs:" outputs)
                             {:d (inc c)})}]}]
    (-> (core/initialize schema {:c 0})
        (core/transact [[[:a] 1] [[:b] 1]])))

so here's what I'm seeing from that

event 2 
inputs: {:c 0} 
outputs: {:d nil}
event 1 
inputs: {:a 1} 
outputs: {:b 1, :c 0}
event 2 
inputs: {:c 1} 
outputs: {:d 1}

it's also a question why event 2 gets triggered before 1

I think it has something to do with the second handler taking 2 inputs, 1 of which is an output from a previous handler

The above breaks

c is nil when the second handler is executed.

Perhaps domino thinks it can run the second handler when it has partial input (i.e. a is set as part of the transaction) instead of waiting until it has a value for c or all dependent handlers have executed.

(-> (domino/initialize schema) (domino/transact [[[:a] 1] [[:b] 1]]) :domino.core/db)

yeah it's looking like the handler gets picked up for the initial transaction with the first set of values

yes, when it should be waiting as it only has partial input

so likely a problem with the way the graph is built then

the handler should get triggered a second time when a new value is generated by the first handler

yes. Weather or not it is triggered the first time is perhaps a matter of opinion. IMO, it should not as the graph should indicate that it will eventually have all inputs (i.e. there is a clear dependency between event one and event two which will result in a complete set of input). But, that is just one opinion. If we run events with partial input, we just need to be explicit about it in the docs as handlers will have to be written to account for this.

i.e. c is nill in the above example.

Either way, there is definitely a bug as the second handler is never invoked with the complete set of input.

I think events being triggered multiple times would mostly affect effectful events

when they're pure then it should be idempotent, and it's just an optimization to ensure they're not being evaluated unnecessarily

but there are cases where you might want to have effectful events, so at least documenting that would be important

I mostly agree. However, the event handler itself will still need to account for the fact that c is nil, for example.

yup

That is what I was referring to.

Just need to make sure that people know they might get incomplete input.

right

but the other issue with the handler not being triggered does need fixing 🙂

and we should add a test for that one for the future

yup.

We have the test, in the code above. 🙂

do you want me to add an issue to the github page?

Slack is not the best place to document things like this. 😉

yeah sounds like a plan

and yeah we should just add the test from here in

One other thing I noticed, which I will include in the issue

Note line 11: This does not actually work as the map that is passed into the handler has a key called :c which has a value of nil. Would it be possible to strip nil values from the input map so that destructuring worked? The effect would still be the same but if we are executing the handlers with partial inputs, it would be nice to be kind to our users and make sure destructuring worked as expected.

https://github.com/domino-clj/domino/issues/24

perfect, and I agree with the destructuring point

I don't think I will have time to look at this in the next 2 weeks (work is pretty busy right now). If it is not already picked up, I'll have a look around then when I come up for air.

for now, I can work around it in my code.

sounds like a plan, if I get a chance to look before then I'll let you know