galeone / tfgo

Tensorflow + Go, the gopher way
https://pgaleone.eu/tensorflow/go/2017/05/29/understanding-tensorflow-using-go/
Apache License 2.0
2.42k stars 154 forks source link

Create error-returning variants of all calls #57

Open ptxmac opened 3 years ago

ptxmac commented 3 years ago

I understand the convenience of just turning errors into panics, it allows easier method chaining and can make the code look much cleaner.

But when integrating into existing codebases, the code is no longer idiomatic and it becomes a bit messy to handle errors without taking down a large system. Of course, it could be handled by wrapping all calls in recover, but that just makes the code very confusing and harder to maintain.

It would be nice if errors handling was a little easier.

E.g.

  m := tg.LoadModelP(....)  // panic on error
  m, err := tg.LoadModelE(....) // return error
  m := tg.LoadModel(...) // does not panic, but saves the error
  err := m.Check() // return above error

It's definitely not perfect, and the idea of having an error hanging around in the model struct isn't the most elegant solution

galeone commented 3 years ago

Hm... I find the proposed solution of doubling the API a bit ugly but I totally understand your point.

Perhaps it would be better to look at the tfgo codebase and see where there are panics that can be turned into errors - for sure not in the method changing (hence every operation on the tensors should panic, and I guess this is correct because it means we are building a static graph in a wrong way and it's just impossible to fix this at runtime).

But from a very quick look, I see that, for example, model.Exec panics, and this is something that instead can return an error (the model can return an error when bad input is passed or other similar I/O problems). Even the LoadModel/ImportModel can both return an error instead of panicking (e.g. when the model location is changed at runtime, it's easy to recover from this situation instead of going in panic).

If you're willing to follow this path (searching for panic in the codebase, check if they are "too much" and replace with the error return, but not in the method changing graph definition ), I'd be happy to review a merge request :smile: