tensorflow / swift-apis

Swift for TensorFlow Deep Learning Library
Apache License 2.0
794 stars 133 forks source link

Add preconditions #517

Open dan-zheng opened 5 years ago

dan-zheng commented 5 years ago

Preconditions (for shape-checking, valid value ranges, etc) would be valuable for better source locations in error messages and to avoid uninformative TensorFlow runtime errors.

Example: matmul(_:_:) currently crashes with an uninformative stack trace.

import TensorFlow
let x = Tensor([1, 1])
print(matmul(x, x)) // invalid
$ swift matmul.swift
Fatal error: In[0] is not a matrix. Instead it has shape [2]: file /usr/local/src/swift-build/tensorflow-swift-apis/Sources/TensorFlow/Bindings/EagerExecution.swift, line 300 # unuseful source location
Stack dump: ...

The performance impact of adding preconditions is not a concern, as long as preconditions are written correctly.

-Onone performance regressions from adding preconditions are acceptable. Preconditions can be disabled with the -Ounchecked optimization mode, e.g. swift build -c release -Xswiftc -Ounchecked.

This blog post has more info on which optimization modes disable assertions/preconditions.

Shashi456 commented 5 years ago

Could we start with making a list of what methods need a precondition ?

dan-zheng commented 5 years ago

Could we start with making a list of what methods need a precondition ?

I'd recommend looking through files in Sources/TensorFlow/Operators and Sources/TensorFlow/Layers and look for "precondition" comments on properties/parameters that aren't actually checked.

Examples:

Before merging patches, it would be good to test the runtime performance impact of adding preconditions.

Shashi456 commented 5 years ago

@dan-zheng I'll try to put up a list in the next few hours

ocampor commented 5 years ago

@dan-zheng I can create a runtime performance analysis on some of the functions with preconditions. Do you have any preferred methodology to assess the impact? Any recommendation to make the analysis more reliable?

Update: Here is a Gist with some quick code to measure the impact. I used assert instead of precondition as it is supposed not to affect performance in shipping code link

If this is okay, I can start a PR including some of the required assertions.

Shashi456 commented 5 years ago

A list of preconditions :

This is just a start and by no means comprehensive, functions like matmul are still to be added and for more we could also look at existing layer implementations in keras and tensorflow for preconditions we might not be checking for.

ocampor commented 4 years ago

Today I’m going to spend some time looking the missing preconditions.

rickwierenga commented 4 years ago

@Shashi456, is your to do list (https://github.com/tensorflow/swift-apis/issues/517#issuecomment-537128724) up to date?

SumanSudhir commented 4 years ago

@Shashi456, is your to do list (#517 (comment)) up to date?

No, it is not updated, preconditions have been added to almost every function mentioned in the list https://github.com/tensorflow/swift-apis/issues/517#issuecomment-537128724

t-ae commented 4 years ago

There's no Layers in list. Can you add them @Shashi456?


Unlike functions, there are multiple choice in Layers where we check.

  1. During init
  2. When parameters are assigned (in didSet)
  3. During callAsFunction

The checks depend on input must go into callAsFunction. The other checks like filter.rank == 4 of Conv2D can be done outside callAsFunction. I think it's good to have them in init.

But, since many parameters are public, users can assign wrong tensor after init. Should we consider that?

If we consider, we have to have same checks done in init in callAsFunction redundantly.

We can have them in didSet and I think it is besically better, but it keeps oldValue for now, so memory consumption will be double. We should wait SE-0268 to be merged.

Shashi456 commented 4 years ago

@t-ae I'll try to add some more functions for this.