Fix TODO in sample.sample_sequences- Avoid 'leaving last token calculation to while loop' #119
Conversation
|
Are you saying the AI was not considering all my context whatttt...huh? When I tested it it was great. What is the new change in English? |
|
Run the initial prompt through the model all at once instead of the final token separately. It's just a tiny performance improvement. |
|
How did you find it? Are you a collaborator or simply you deeply analyzed the code of GPT-2? I'm interested in paying someone a lot to understand GPT-2...let me know... |
src/sample.py
Outdated
| def body(past, prev, output): | ||
| next_outputs = step(hparams, prev[:, tf.newaxis], past=past) | ||
| def body(past, prev, output, first=False): | ||
| next_outputs = step(hparams, prev if first else prev[:, tf.newaxis], past=past) |
There was a problem hiding this comment.
how about we just always make it just prev, and don't do the later squeeze?
src/sample.py
Outdated
| logits = next_outputs['logits'][:, -1, :] / tf.to_float(temperature) | ||
| logits = top_k_logits(logits, k=top_k) | ||
| samples = tf.multinomial(logits, num_samples=1, output_dtype=tf.int32) | ||
| return [ | ||
| tf.concat([past, next_outputs['presents']], axis=-2), | ||
| next_outputs['presents'] if first else tf.concat([past, next_outputs['presents']], axis=-2), |
There was a problem hiding this comment.
i think this should probably be if past is not None instead of if first
src/sample.py
Outdated
| ] | ||
|
|
||
| past, prev, output = body(None, context, None, first=True) |
There was a problem hiding this comment.
i would remove the first=True flag, and change output to just be tf.zeros([batch_size, 0]) or something like that
There was a problem hiding this comment.
got it, I was considering that. Mine is simpler to read, but yours is kinda like just better.
There was a problem hiding this comment.
Ok, so this was actually the source of the bug. We need to pass the context as the output.
merge fix sample.py TODO
|
In order to set the seed, I made this single change (not sure how Fire works). Then just called 'python interactive_conditional_samples.py' on the 117M model. And this is from my current fork (which has pulled from upstream): Are there more random seeds to be fixed? |
|
tensorflow seeds are dependent on the graph :( silly design |
|
hmm, how do i fix that? |
|
I would just test with k=1 or something. your outputs definitely look buggy though also |
|
Maybe. not 'buggy' as I think I did exactly what the TODO requested. I'll take a look at the output, and re-check that the TODO request should not change the output tho. |
|
Remember this is 117M. Second output looks fine. |
|
Actually, is there anyone you can ask about the TODO request? I do not think it would change the output. |
|
Oh yeah you do mean top_k, nvm. Thanks for the tip! |
|
To put it very briefly, the TODO does not change the output because all Decoder states from past iterations are passed to the current iteration. |
|
Close but not quite. My fork: Same prompt as above. Seems like just a bug, or at least fixable. Missing output in the beginning, but managed to get the exact same to repeat. I'll run more tests. |
|
Wait, about your 'depends on graph' comment. I did not make your suggested change yet, I am testing with this, see my fork for the exact repo I am using, besides the top of the file, my version here: |
|
Ok yes, it seems like my fork is completely correct except for missing the non-repeating output. The prompt: |
|
Damn the model took the next few words right out of the article: https://www.theverge.com/2019/5/2/18525323/detective-pikachu-review-pokemon (jk obvs article released after). I'll get to looking into the bug with my fork. |
|
Testing one more time with the prompt as 'hi' orig |
|
Juuuuust in case, tested on Unconditional, with same seed fixes at the top of file: |
|
Certainly let me know if you or someone at OpenAI finds out why! I'm working on it too. |
|
To summarize the above, I am testing my fork with the TODO fixed, against the actual repo, with the seeds fixed and top_k=1. It seems to match exactly, except for missing some of the output in the beginning. |
|
Ok, comparing my code with the original, I think I see where I had a bug. Will fix and test. |
|
I will work on removing the 'first' parameter. |
|
Ok, tested after removing first param, same result: P.S. after testing on the Detective Pikachu quote, I noticed that since I had added a space to the end of the prompt, I got a different result. Without the space actually gives a better result. |
|
Tested on the 3 prompts, plus Unconditional. Results of this fork are the exact same as the actual repo. top_k set to 1 and seeded with 1957, in addition to adding to the top of the script just in case. |
|
The fork is complete. Barring any further tests you may have. Output matches exactly. |
|
Just curious, may some of the changes (like shape invariant) cause complications for library users? Or have you just not gotten to looking and checking yet? |
|
I can write a test script with the two versions of sample.py. Where should I put it? Do you have an exact way you want me to go about it? |
|
ah sorry, didn't get around to looking for awhile. looks good! |
…ation to while loop' (openai#119) * do initial run on full context * decrement while loop iterations * add context to output * remove first param * removing first param: change shape invariant
Hi,
This change runs the initial model step on the full context, by calling the body() function. I added an 'first' parameter defaulting to False to allow this.