Skip to content

Conversation

@rdentato
Copy link
Contributor

@rdentato rdentato commented Aug 7, 2023

Rather than exit, let's treat rng_seed = 0 as an indication for a default value (time(NULL)).
It seems more reasonable to me ...

@xefoci7612
Copy link

xefoci7612 commented Aug 7, 2023

@rdentato this is not deterministic

If we run 2 times

./run model.bin -s 0
./run model.bin -s 0

We'd like to see the same output (this is the reason we use the -s option)

IMHO a correct patch that preserves deterministic behavior can be

else if (argv[i][1] == 's') { rng_seed = atoi(argv[i + 1]) ^ RND_0; }

where for instance

#define RND_0 0x2545F4914F6CDD1Dull

and we don't need to special case the 0 anymore

@rdentato
Copy link
Contributor Author

rdentato commented Aug 7, 2023

The issue I see with

rng_seed = atoi(argv[i + 1]) ^ RND_0;

is that if RND_0 is 0x2545F4914F6CDD1Dull then

./run model.bin -s 2685821657736338717

will set rng_seed to 0.

Yes, is very unlikely, but still possible.

The reason behind my proposal is that you can imagine run called by a script

   ./run model.bin -s ${LLM_SEED:-0}

If LLM_SEED is not set, the default value is given (and it's a value that changes all the time).
I usually prefer to have a value that signifies default and 0 as a seed seems a very good candidate to me.

@twobob
Copy link

twobob commented Aug 8, 2023

If LLM_SEED is not set, the default value is give (and it's a value that changes all the time). I usually prefer to have a value that signifies default and 0 as a seed seems a very good candidate to me.

Exact reason s=0 was immediately tested here, following the change. Concur, fwiw, with this mindset.
That said, determinism was also our expectation so, unsure.

@karpathy karpathy merged commit 04121d1 into karpathy:master Aug 10, 2023
@rdentato rdentato deleted the patch-rng-seed branch August 10, 2023 15:22
vinhtran2611 pushed a commit to vinhtran2611/llama2.c that referenced this pull request Jan 20, 2024
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

Successfully merging this pull request may close these issues.

4 participants