srush / llama2.rs

A fast llama2 decoder in pure Rust.
MIT License
1.01k stars 56 forks source link

Minor Nitpics, from an also rust newbie :) #5

Open cchance27 opened 1 year ago

cchance27 commented 1 year ago

Try to normalize your naming convention for variables in places like structs if possible, in a cursory look I noticed your doing the thing math guru's do with x, xb, xb2 etc and then commenting the actual name next to it, really muddies up the interfaces and consuming the structs, and just reading things...

For instance hitting, let x = &mut s.x;, sure... umm... oh yah it's the runstate's ... x.... wtf was x again, oh yah let me scroll up and check the comment for it, ahh x is the current activation :) Having to scroll back and forth really makes reading the code more challenging, and leaves more room for errors to be made, especially with throw away variable naming :)

It seems pretty safe to do a #[inline] on the various helper functions so the compiler is more likely to inline them, will prevent a jmp when it compiles down (it might already get inlined but doesn't hurt to give the hint).

I was actually looking to get into inference and understanding it more, will definitely dive into your code more tomorrow to try to understand it more, but looks really interesting just a bit confusing coming into inference, with the naming and commenting style, really great to see more people taking rust on for ML tasks.

srush commented 1 year ago

yeah, I agree with all this. The names and coding conventions came directly from https://github.com/karpathy/llama2.c I just copied them over. I'll do another pass to make the names better and remove all the unneccessary comments that are now type checked in rust.