grailbio / reflow

A language and runtime for distributed, incremental data processing in the cloud
Apache License 2.0
965 stars 52 forks source link

reflow/syntax: digest exprexec in stdEvalK #128

Closed prasadgopal closed 3 years ago

prasadgopal commented 3 years ago

If an exec has delayed dependencies, we need to digest them while computing the flow digest. Without this, execs with delayed dependencies can result in having same logical digests, which is incorrect.

No tests yet. will add some. Also i feel like we should cover all the cases here that are relevant and panic for cases which are no supposed to use this (like compr).

mariusae commented 3 years ago

I think your other fix is the correct one: namely, to evaluate these when constructing the k for exec in the first place. Evaluation here only means to evaluate it to a flow, so I'm not sure why the code was like that in the first place. (Since this will never cause any additional flow evaluation. Admittedly, I probably wrote this code, and my mind must not have been completely sorted at the time.)

Let me try to explain the bug etiology as far as I can tell. First of all, the example is reducible:

param alternate bool

val f1 = file("/tmp/f1") // f1 contains "alice"
val f2 = file("/tmp/f2") // f2 contains "bob"

val theFile = if !alternate { f1 } else { f2 }

func fn(f file) (out file) =
  exec(image := "ubuntu") (out file) {"
    # {{delay("x")}}
    cp {{f}} {{out}}
   "}

func fn2(f file) (out file) =
  exec(image := "ubuntu") (out file) {"
    cp {{f}} {{out}}
   "}

val a = fn(theFile)
val b = fn2(a)

@requires(cpu := 1)
val Main = (a, b)

Note that the only thing that is required is that the first exec renders a non-file, non-dir delayed argument. i.e., the exec triggers a continuation.

So, what's happening here is that when the K node is constructed, as you note, the file and dir arguments aren't captured, only the non- file and dir arguments!

They should be evaluated -- it makes no difference (from the point of view of flow evaluation) whether they are evaluated before the K or after.

We should probably audit for other such instances as well.