-
Notifications
You must be signed in to change notification settings - Fork 363
Making results independent from threadcount/batch size (from llama.cpp) #67
Comments
Seems like this is rearing its head again: ggerganov/llama.cpp#603 (comment) |
Interesting... not sure what to make of that. I suppose we could do more performance testing: I wonder if #87 (comment) is related? |
I did some more testing with combinations of 6 and 12 threads, 16 and 32 bit memory and ID on or off. This was with a 7B Alpaca model, generating 80 tokens and just timing the total run time. It was pretty consistent though, and I did the ID (increased_determinism) one three times just to be sure. Sorted by type:
Sorted by TPS (higher is better):
In the TPS sorted version, ID off comes out ahead pretty consistently. It's not a massive difference most of the time. I don't know why ID + 6 threads + 32bit memory looks so bad but those results were consistent. Anyway... It probably would make sense to go back to the behavior with the commandline argument that toggles it with it automatically getting enabled when a seed is specified. The difference is definitely enough to care about in the 32bit memory/6 thread case. Not sure if it's just a thing with low numbers of threads in general or what. |
Can anyone else replicate the performance differences I saw? If so, do we want to go back to the previous proposed behavior where there's a CLI flag which gets automatically enabled when |
Sorry, haven't had the time to test. Seems like you've done some pretty thorough testing and other people have reported regressions in upstream, so I'm leaning towards your suggested solution of apply-only-when-seed-is-specified. I'll reopen this issue for now... |
I think this is the same thing, right? I'm guessing people would want to use that approach rather than trying to invest time in making the existing on better. edit: I've been testing the changes. Not 100% sure it's all this change, but something changed recently that made a big difference. |
Yes, that seems related. Looks like we'll need to sync with them soon... |
This may be something to keep an eye on: ggerganov/llama.cpp#439
Looks like the corresponding code is here: https://github.com/rustformers/llama-rs/blob/bf7bdbcfff3114dcbdafb6eb7eed58f04f19b1c3/llama-rs/src/lib.rs#L1203
According to the comments in the pull, it should trade a small amount of performance for less memory usage. However, at least one user commented they saw more memory use (not sure what size model).
The text was updated successfully, but these errors were encountered: