wilkerlucio / pathom3

Interface with complex data via graph mapping.
https://pathom3.wsscode.com
Eclipse Public License 2.0
376 stars 31 forks source link

Unnecessary resolver call #134

Closed nivekuil closed 2 years ago

nivekuil commented 2 years ago
(pco/defresolver test [{:keys [id]}]
  {::pco/output [:link :payload]}
  {:link "foo" :payload "bar"})

(pco/defresolver test-b [{:keys [link]}]
  {::pco/output [:resp]}
  (println "hi")
  {:resp "resp"})

(pco/defresolver test2 [{:keys [id link resp]}]
  {::pco/output [:payload]
   ::pco/priority -1}
  (println "hi2")
  {:payload "bar2"})

@(p.a.eql/process (pci/register [test test-b test2]) {:id 1} [:payload])

test-b is being called, printing "hi", even though test has enough information to resolve the query. test2 is not called.

wilkerlucio commented 2 years ago

@nivekuil looks correct behavior for me, the test2 says it requires :resp as an input, so Pathom has to call test-b to get :resp to fulfill the test2 input requirements.

Makes sense?

nivekuil commented 2 years ago

test2 is completely unnecessary here though, only test is needed, and preferred because of priority. :payload is "bar"

wilkerlucio commented 2 years ago

Ah, right, ok, let me take a deeper look at it.

wilkerlucio commented 2 years ago

Ok, I see the problem, the cause is due to some node optimizations, OR nodes always proving to be tricky.

This is what the unoptimized version of the graph looks like:

Screen Shot 2022-04-01 at 13 48 30

After some optimizations we get to this crucial point:

Screen Shot 2022-04-01 at 13 49 02

From there Pathom optimizes to this (which is the final graph):

Screen Shot 2022-04-01 at 13 49 57

Now, as you can see, because one of the paths is a subpart of another path, they get merged, in those cases, it's expected that the whole trail won't have to run because the data is going to be ready before that. Pathom checks if we have the data already before calling a resolver, by looking at the ran graph we can see that test2 doesn't get called in this scenario:

Screen Shot 2022-04-01 at 13 51 10

The problem is the test-b in between, since its a transitional dep Pathom still ends up calling, now it's about thinking how to deal with this situation, the first idea that comes on top of my head is check for query resolution after each step, but this would add some overhead, so I like to take a time to see if there are other options, I'm also open for ideas on it.

wilkerlucio commented 2 years ago

Tip: in case you are unware, you can see all of this yourself in Pathom Viz, this is the code I did to test with your example:

(ns com.wsscode.pathom3.demos.repro-calls
  (:require [com.wsscode.pathom3.connect.operation :as pco]
            [com.wsscode.pathom3.interface.async.eql :as p.a.eql]
            [com.wsscode.pathom3.connect.indexes :as pci]))

(pco/defresolver test [{:keys [id]}]
  {::pco/output [:link :payload]}
  {:link "foo" :payload "bar"})

(pco/defresolver test-b [{:keys [link]}]
  {::pco/output [:resp]}
  (println "hi")
  {:resp "resp"})

(pco/defresolver test2 [{:keys [id link resp]}]
  {::pco/output [:payload]
   ::pco/priority -1}
  (println "hi2")
  {:payload "bar2"})

(def env
  (-> (pci/register [test test-b test2])
      ((requiring-resolve 'com.wsscode.pathom.viz.ws-connector.pathom3/connect-env)
       "debug")))

(comment
  @(p.a.eql/process env {:id 1} [:payload]))

Then, running the query, on the history tab we can see the whole query, and by clicking on the graph there is a button to Log Snapshots, where you can see the step by step process taken to build the graph:

Screen Shot 2022-04-01 at 13 56 00

I hope this also help you to find and see how things are happening.

nivekuil commented 2 years ago

I do use pathom viz but did not know what the log plan snapshots button did. Thanks!

wilkerlucio commented 2 years ago

@nivekuil A fixed just landed on main (0ae92361326c341bbffe832042a8929a92b59153), it fixes the repro you sent. Can you try this fix on your actual source problem and verify that it works as expected?

nivekuil commented 2 years ago

Seems to work, do you expect priority to matter here though? It seems no matter the priority of test, it will always be preferred over test2

wilkerlucio commented 2 years ago

Priority won't matter in this case, priority is decided during the running process, since no OR remained in the plan, the priority won't be taken into account.

wilkerlucio commented 2 years ago

Fixed by 0ae92361326c341bbffe832042a8929a92b59153