Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Math is difficult to implement #691

Open
timokoesters opened this issue Aug 31, 2023 · 16 comments
Open

Math is difficult to implement #691

timokoesters opened this issue Aug 31, 2023 · 16 comments

Comments

@timokoesters
Copy link

In Python it's possible to write just this:
3 * x**2 - 4*x + 5

With Candle it currently looks like this, which is difficult to read and write:
Ok((&(&(&x.powf(2.0)? * 3.0)? - &x * 4.0)? + 5.0)?)

  1. Why does everything return a Result? I assume the reason is that the GPU connection might fail, but in that case maybe it's okay to panic the whole application?
  2. Why is Tensor not Copy? It's an Arc, so it would be okay in theory.
  3. A few more traits can be implemented to make it look like this: 3.0 * x * x - 4.0 * x - 5.0

I wrote an example here:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=e5abf73c9e38fb6cef2c17b5a1898a29

@LaurentMazare
Copy link
Collaborator

Yes the syntax is cumbersome, we have some ideas that should slightly improve it though it will not be as simple as the version you suggest. To answer your points:

  1. All the ops may indeed fail, the most common thing is not really the GPU going down but more the GPU running out of memory, using invalid shape, etc.
  2. I don't think a Tensor could be copy, copy implies that you can just do copy the immediate memory representation (see here) which isn't the case for Arc. Hence Tensor is only clone - and the clone op is pretty inexpensive.

@timokoesters
Copy link
Author

One idea to work around results is to impl Add<Result> for Tensor, to allow chaining the results automatically, but I don't know if that is a great idea.

And I think instead of deriving Copy, you can impl operations both on the values as well as on the references (or AsRef?)

@LaurentMazare
Copy link
Collaborator

Both of these are actually already done :)

@timokoesters
Copy link
Author

Oh that's great, maybe I was using an old version, or it only worked one way.

@timokoesters
Copy link
Author

timokoesters commented Aug 31, 2023

It looks like Tensor + Result is implemented, but not Result + Tensor or Result + Result. It also doesn't work for methods like broadcast_div.

I have another related problem: I'm unable to write a rusty variant for modifying a single index:
var[2, 3] += 1, is there a method that can help me with that?

@LaurentMazare
Copy link
Collaborator

Re Result + Result I think this is because of the foreign trait limitation of rust so not much that we can do here.
Re assigning single values in place, we don't really have an op to do this and it's on purpose as these tend to be really inefficient so we prefer favoring more functional approaches and avoiding accessing data by index. I don't think this has been an issue for all the models we've been looking at so far but if there are some specific models for which this would be a limitation, certainly curious to hear about them.

@timokoesters
Copy link
Author

timokoesters commented Aug 31, 2023

Sorry, you are correct on the Result+Result, but the other combinations will work, as you can see in my example. An idea to even allow result+result is to have all operations return Tensor, but hide the Result internally and only expose it with some getter methods.

I am redoing the "Neural networks from zero to hero" with Candle and one of the first things is building a character combination count matrix (https://youtu.be/PaCmpygFfXo?feature=shared&t=574). This adds 1 to specific entries in the matrix. Another use case would be implementing one-hot encoding.

Anyway, thanks for your quick answers!

@timokoesters
Copy link
Author

most common thing is not really the GPU going down but more the GPU running out of memory, using invalid shape

These sound like unrecoverable errors, so it could be okay to panic in these cases?

@chronicl
Copy link
Contributor

chronicl commented Aug 31, 2023

Making tensors Copy could be achieved by having some global state hold all the information of the tensors and making the tensors themselves ids for retrieving that information. This is commonly done in rust whenever you want a type to be Copy for ergonomic reasons, for example in leptos's reactive systems. Of course this comes along with all of the caveats of dealing with global state.

I do like this idea:

hide the Result internally and only expose it with some getter methods.

this would allow for more ergonomic math and allow users to choose when they want to return early due to an error (if at all). I believe Rust's track_caller can be used to track the location of where the error actually happened, so we wouldn't lose that information.

@LaurentMazare
Copy link
Collaborator

most common thing is not really the GPU going down but more the GPU running out of memory, using invalid shape

These sound like unrecoverable errors, so it could be okay to panic in these cases?

I don't really agree with this. Imagine you have a server that runs inference and somehow your gpu runs oom, I feel that you would want some proper error handling at this point so that you can stop the current computation but not the whole server, (and catching the panic to do this would sound sketchy).

@Narsil
Copy link
Collaborator

Narsil commented Sep 1, 2023

100% on this. Having implemented a few servers, panicking is not great !

@cchance27
Copy link

Trying my first hand at math with candle, and just ran into something... is Add only available really for f64? Not u32, etc?

@LaurentMazare
Copy link
Collaborator

Trying my first hand at math with candle, and just ran into something... is Add only available really for f64? Not u32, etc?

Right, it's just a convenient shortcut. If you want to add an u32, you can do something like tensor.broadcast_add(&Tensor::new(42u32, tensor.device())?)?.

@cchance27
Copy link

Not gonna lie, I’m sorta an idiot as I figured I could use broadcast_add but then was like “wtf it takes a tensor, not a u32…. I’m so confused… completely forgetting I could just create a single value tensor lol

@asukaminato0721
Copy link

Oh, I currently use tensor.affine(1., 42.)? lol.

@timokoesters
Copy link
Author

timokoesters commented Aug 27, 2024

I've worked on a wrapper type around Result called MTensor ("Maybe Tensor") that allows simpler math: https://github.com/timokoesters/candle/blob/6d4bc204ad95e6ed1e51f6679504e8ce2d1ee210/candle-core/src/mtensor.rs#L212-L222

@LaurentMazare Can you take a look? Is that something that could be merged into Candle?

PS: It relies on this unstable feature rust-lang/rust#84277, hopefully it will work in stable rust soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants