Skip to content

Conversation

@tafsiri
Copy link
Contributor

@tafsiri tafsiri commented Mar 18, 2020

This moves the definition of inputsToSave and outputsToSave for gradient computation onto the gradient config.

runKernelFunc still takes inputsToSave and outputsToSave as fallbacks for ops currently using those directly. However those two params to runKernelFun are effectively deprecated.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

@tafsiri tafsiri marked this pull request as ready for review March 18, 2020 20:37
@tafsiri tafsiri requested review from annxingyuan and nsthorat March 18, 2020 20:37
Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @nsthorat, and @tafsiri)


tfjs-core/src/engine.ts, line 578 at r1 (raw file):

              this.getTensorsForGradient(kernelName, inputs, outTensors);
          if (tensorsToSave == null) {
            // Fallback for WASM kernels that use inputsToSave and outputsToSave

what does this comment mean? wasm is linked at master so you can update it here if it's something about WASM


tfjs-core/src/engine.ts, line 666 at r1 (raw file):

      outputs: Tensor[]): Tensor[]|undefined {
    const gradConfig = getGradient(kernelName);
    if (gradConfig) {

!= null


tfjs-core/src/engine.ts, line 678 at r1 (raw file):

    // TODO(yassogba) throw exception here once all inputsToSave/outputsToSave
    // are removed
    return undefined;

use null instead of undefined here

@tafsiri tafsiri merged commit 6da3fa8 into master Mar 19, 2020
@tafsiri tafsiri deleted the gradients-inputs-to-save branch March 19, 2020 03:12
@tafsiri
Copy link
Contributor Author

tafsiri commented Mar 19, 2020

@nikhil. Sorry I forgot to hit publish in reviewable before merging. All fixes have been done. Copying my reviewable comments here.

what does this comment mean? wasm is linked at master so you can update it here if it's something about WASM

Good catch, this was confusing, I updated the comment to be more precise/corrent. Some ops call runKernelFunc and pass inputsToSave and outputsToSave, however it looks like this was done just for ops that were implemented in WASM backend. So this comment was meant to be a breadcrumb to remember why this codepath exists. I think it was done in anticipation of gradient registration but isn't needed once we change those ops.

!= null

Done

use null instead of undefined here

Done

@nsthorat
Copy link
Contributor

Makes sense, thanks for the explanation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants