swift-apis
swift-apis copied to clipboard
Add preconditions
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.
Could we start with making a list of what methods need a precondition ?
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:
-
func conv2D
,/// - Precondition: `input` must have rank `4`.
- Missing a precondition at top of function.
-
Conv1D.filter
,/// The 3-D convolution filter.
- Missing precondition in initializer.
- Other shape-related functions:
matmul
,reshape
, etc - Probability-related APIs (
Tensor.droppingOut
,Dropout
layer) should have0..<1
preconditions for probability values.
Before merging patches, it would be good to test the runtime performance impact of adding preconditions.
@dan-zheng I'll try to put up a list in the next few hours
@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.
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
andisAlmostEqual
- [ ] 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.
Today I’m going to spend some time looking the missing preconditions.
@Shashi456, is your to do list (https://github.com/tensorflow/swift-apis/issues/517#issuecomment-537128724) up to date?
@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
There's no Layer
s in list. Can you add them @Shashi456?
Unlike functions, there are multiple choice in Layer
s where we check.
- During
init
- When parameters are assigned (in
didSet
) - 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 I'll try to add some more functions for this.