-
Notifications
You must be signed in to change notification settings - Fork 214
Framework: Move Ops parameter to call method where possible #202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
@JimClarke5 , we've been discussing Ryan and I about this and would like to revive this proposal, whenever you're ready |
Actually, this may help with some of the stuff I am doing for Model. The Model creates the graph, so it is difficult to create an Optimizer first, until the Model does this. Right now I have Model using Lambda functions to create Loss, Metrics and Optimizers after the Model creates its Graph/Ops. I am open to a way to defer setting the Ops (or whatever) later on in the Object's life cycle. BTW: I think we should revisit Optimizers to be consistent with the rest of the framework packages. |
Optimizers will probably want to (only?) support the new variables, so you may want to wait for that. It would be nice if there was an option to pass in a list of variables to optimize rather than checking the graph, for eventual eager support. |
Some of the classes create internal |
I agree w/ Something like: Variable<TFloat32> w = newVariable("w", Shape.of(10, 10));
onInit(() -> w.initWith(...)); Most of the variable stuff should be handled by the new API (using init scopes), once the gradient makes it into tensorflow core, but if you want to apply a initializer to a variable or something it's still good to have that step. |
This applies to |
Should we create an interface,
|
Also, I think we would have to do all the ctors in framework that take an |
It would be nice to not have to expose it to the user, i.e. something like: abstract class Activation {
private List<Consumer<Ops>> initers = new ArrayList<>();
private boolean inited = false;
protected final void onInit(Consumer<Ops> onInit){
initers.add(opInit);
}
protected void init(Ops tf){}
public final Operand<TFloat32> call(Ops tf, Operand<TFloat32> x){
if(!inited){
init(tf);
initers.forEach((x) -> x.call(tf));
inited = true;
}
return doCall(tf, x);
}
abstract Operand<TFloat32> doCall(Ops tf, Operand<TFloat32> x);
} where the init stuff gets called on the first |
So with the call proposal, what is the expected behavior the 2nd time call is invoked? |
Right, duh, there should be a flag, edited it. The init logic should probably be moved to a synchronized method. |
I do not favor passing
My preference is |
I don't see the issue with 1, it's still re-creating all the operations, just in the same scope or a new one. And since you're passing in operands you almost always have the Ops available. 2 is exactly why I wanted Ops in the call method, since with Keras, you can do something like create a For 3, I envisioned having Also, I would like to separate the "Ops in call" from "init" a little bit. It's much nicer for the Kotlin API if we can make some of the commonly customized Framework classes (Activation, Loss) functional interfaces, and for the most part they don't need initialization, so you could have something like |
FYI, It is not just |
Hmm, yeah, it seems like there's a definite dichotomy between stateful/graph linked classes like |
The other option would be to always use eager variables for the state, like is done in functions. But again, I don't think that's worth doing for the few stateful classes. |
The more I think about it, passing |
So these packages are stateless with respect to the TensorFlow graph; The stateful packages are One thing I am seeing is that sometimes the code is more concise. if you also have an optional
versus
The second case could call |
Why would you need to return an op, couldn't you just use It does prevent pre-creating layers, i.e. outside of a Model and then passing them in, but that only matters if the Graph is tied to Model (I haven't looked at the API yet). For the stateful packages (mostly Layers), did you plan on adding an I haven't looked at the Layer API details yet either, but how do you plan on handling weight sharing? Keras does it by re-using the Layer objects, and something similar would be nice here, which works well with taking the init |
I don't see Variable initialization is a pita. I was not planning on passing Ops to the layer call method, because layers have to initialize components like optimizers and metrics before processing. It would be possible to invoke I haven't run across weight sharing yet, but I do note that a variable with a unique name is sharable through out the Graph. My current There are some things in TF Python, Action Item for me: I am going to research a way to add a control dependency to a variable for the initialization phase so that it only exists the first time it is used, and not subsequently. |
One other wrinkle is that the initialisations are only populated in the |
I ran into the initialization stuff as well when working with functions and the new variables, see #238 and #237. I'm still waiting for the variable gradients in core to get released, but I can do the init scope sooner if you could use it. The same-name trick will likely only work with the old variables, not resource variables, and only exists atm because of bugs in NameScope (it should be prevented, since any other name-clashing operations will error). |
I have converted the metrics to try and make sure dependency operations are on operations when the variables need initialization. I have this working, but there are some issues.
Any suggestions would be appreciated. I have changed the |
I don't like using control dependencies for init, for the reasons you mentioned. It doesn't play nice with sessions either, even if you don't re-use the operand (it can re-init the variable each time the session runs if you aren't careful). Honestly, I'd wait for the tf.Variable and init scope APIs to do the stateful classes, I'm not sure how you would do it otherwise. Also, the op you probably want instead of select is |
@JimClarke5 here's what I was looking at for Kotlin: https://github.com/rnett/weaver/blob/main/src/main/kotlin/com/rnett/weaver/Module.kt#L95 You would need to specify keys for remembering stuff in Java, and I'm not sure how you would ensure init is called. |
I'd like to move the
Ops
parameters of framework classes to the call method, where possible. This is primarily for Kotlin interop, but has a few other benefits as well. It won't be possible for stateful classes (Metrics, Optimizers), but should be possible for most, as far as I can tell (Initializers, Activations, Losses). I'm going to use Losses as a standin for all 3 for my examples.@FunctionInterface
. If we do this, we can then define new losses likeLoss{ tf, x -> tf.stuff(x) }
or pass lambdas to methods that take losses. This is very nice for layers, where we might have something Keras-likeLayer(activation=ReLU())
but want to replace it with something custom.Ops
for (eager) tensor lifetime management, which has been suggested and is something I'd like to do (it's easy enough to make a long-lived copy of the initial scope, but then the tensors created in the call methods live forever, and the framework classes need to be closable).Layer(activation=LeakyReLU(alpha=0.3))
. I expect this will be common with our API, as well. Currently, this runs into the above issue w/ scoping, and prevents you from passing activations (or losses) from scopes that don't have an Ops instance available.I'd look at having the stateful classes take
Ops
incall
as well, and only using the constructor ops for initializing state. This works better with scoping and lifetimes as mentioned above.The text was updated successfully, but these errors were encountered: