axon icon indicating copy to clipboard operation
axon copied to clipboard

Warn on non-MFA functions

Open seanmor5 opened this issue 2 years ago • 5 comments

Resolves #323

For now, just warns. Will raise on next release

seanmor5 avatar Sep 06 '22 22:09 seanmor5

@seanmor5 there is a limitation in this approach in that you cannot access closures. For example, imagine someone does a layer like this:

y = 1
layer(x, fn x -> x + y end)

If we force external functions, there is no way to access y anymore. Maybe that's intentional, maybe you require all bindings to be given as layer arguments like this:

y = 1
layer({x, y}, fn {x, y} -> x + y end)

But in case it isn't, you probably want to revisit. :)

josevalim avatar Sep 07 '22 08:09 josevalim

@josevalim I think that's okay because you still wouldn't be able to deserialize the closure correctly in another program?

In cases where you want more flexibility, you can just serialize the parameters and ensure you use the same code to generate the model programmatically

seanmor5 avatar Sep 07 '22 10:09 seanmor5

Yes, the closure is wrong either way. My concern is that we cannot reproduce the closure capabilities now, with the chosen API, unless we support an additional optional argument, called extra_args, which we append to the existing args. So, for example, we could do:

layer(x, &SomeFun.bar/4, [y])

josevalim avatar Sep 07 '22 10:09 josevalim

There is a workaround for this which I just pushed, but as you can see it is failing because there are some inconsistencies in generating parameter shapes. Basically if you want a closure you can just use Axon.constant layers as input :)

The issue now is that parameter shapes are functions which depend on the known input shapes at initialization time, which creates kind of weird scenarios where if you swap out a parameter for a constant than it breaks things. I think I need to update that to work regardless of whether a layer input is an Axon input or Parameter input

seanmor5 avatar Sep 07 '22 11:09 seanmor5

Perfect then, ship it!

josevalim avatar Sep 07 '22 11:09 josevalim