swift-apis icon indicating copy to clipboard operation
swift-apis copied to clipboard

Add preconditions

Open dan-zheng opened this issue 4 years ago • 10 comments

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.

dan-zheng avatar Sep 29 '19 14:09 dan-zheng

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

Shashi456 avatar Oct 01 '19 06:10 Shashi456

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.

dan-zheng avatar Oct 01 '19 06:10 dan-zheng

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

Shashi456 avatar Oct 01 '19 06:10 Shashi456

@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.

ocampor avatar Oct 01 '19 08:10 ocampor

A list of preconditions :

  • [x] Preconditions for conv2d for rank and filter
  • [x] Preconditions for conv3d for rank and filter
  • [x] Preconditions for depthwiseconv2d for rank and filter
  • [ ] Precondition for unstacked for axis
  • [ ] Preconditions for split for count and axis
  • [ ] Precondition for tiled
  • [ ] Preconditions for all reshaped functions
  • [ ] Precondition for concatenated
  • [ ] Preconditon for gathering
  • [ ] Precondition for batchGathering [Note: there are a few preconditions whih have been commented out]
  • [ ] Precondition for broadcasted
  • [x] Precondition for unbroadcasted
  • [ ] Preconditions for slice
  • [ ] Preconditions for elementsAlmostEqual and isAlmostEqual
  • [ ] Preconditions for replacing
  • [ ] Preconditions for all, any
  • [x] Preconditions for min, max, argmax, argmin
  • [x] Preconditions for sum, product, mean, variance, cumulativeSum, cumulativeProduct
  • [x] Preconditions for standardDeviation, logSumExp
  • [x] Preconditions for moments

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.

Shashi456 avatar Oct 01 '19 16:10 Shashi456

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

ocampor avatar Nov 15 '19 09:11 ocampor

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

rickwierenga avatar Jan 31 '20 11:01 rickwierenga

@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

SumanSudhir avatar Feb 03 '20 10:02 SumanSudhir

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.

t-ae avatar Feb 19 '20 03:02 t-ae

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

Shashi456 avatar Feb 19 '20 05:02 Shashi456