Bug Fixed GPTNeoX Flax supports#25334
Conversation
sanchit-gandhi
left a comment
There was a problem hiding this comment.
Looking super clean already @HeegyuKim! Mainly just some very minor comments from me - the overall design is great. Once these are addressed and the tests pass we can get it ready for merge 🚀
|
I suffering from test issue. Can you help me? @sanchit-gandhi summary
I don't think this is a problem with my model implementation. I wonder why pytorch's test fails. This PR failed two tests below But two flax tests in tests/models/gpt_neox/test_modeling_flax_gptneox.py are fine. the test code which was copied from #24002 override both test_equivalence_pt_to_flax and test_equivalence_flax_to_pt methods with this comment. and they use below assert code instead of This overrides are equal to |
|
Hey @HeegyuKim - could you confirm that you get the same logits out from the Flax model when you generate as with the PyTorch model? i.e. that the generation scores are the same in both cases. If this is indeed the case, then we can know for certain that the Flax implementation is correct, and that we need to override the PT-FX cross tests. Otherwise, there's a divergence that we need to fix! We can check this with the full GPT NeoX model to ensure we have the right logits here |
|
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
|
Hey @HeegyuKim! I thought a little bit more about the PT-FX cross tests in the [WIP] Flax LLaMA port with @vvvm23, and suggested that probably the reason for the failing tests is the random attention mask: #24587 (comment) If we switch to using a causal attention mask, we are able to get PT-FX equivalence for Flax LLaMA without overriding the tests. Since Flax LLaMA is heavily based off Flax GPT-Neo, I'm fairly certain we'll observe similar behaviour for Flax GPT-NeoX Would you like to try running the tests using a causal attention mask? E.g. as per #24587 (comment) |
|
Hi! Thanks @HeegyuKim for the PR. I am wondering if there is any update on this? It would be really cool if we could use GPTNeoXForCausalLM in flax! |
|
Hello @liutianlin0121 I'm trying to solve the problem whenever I have time. However, even if causal masking is applied, the error in the model output is still larger than 1e-5. The current error is around 0.02-0.03. I'm going to try again this weekend. Even though there are errors, the model works better than expected. I trained several models with this code.
I want to contribute to huggingface but it's not as easy as I thought. |
There was a problem hiding this comment.
It looks like you're making solid progress @HeegyuKim! Nice work! The most recent CI run is reporting that the difference in the Flax and PyTorch model outputs is > 1 (see CI Output). This suggests to me that there is a divergence between the Flax and PyTorch models. Generally, for any model less than 1B params, we should be able to get equivalence to within 1e-5 between Flax and PyTorch. It's quite likely that you won't get this equivalence running the matmuls in bfloat16 on TPU. But you should be able to running the matmuls in float32, see #15754 and jax-ml/jax#10413 (comment) for details
Here's a script that I used previously for checking PT / Flax equivalence for BLOOM: https://github.com/sanchit-gandhi/codesnippets/blob/main/check_flax_bloom_jit_small_testing.ipynb You can ignore the bits about JIT'ing the forward pass for the time being. You can also uncomment the check to run it on CPU to force the highest precision, or use the decorator as provided
If we don't get 1e-5 precision, it's usually an indicator that we have a divergence in our model. Here, going through layer-by-layer and checking the hidden-states might be required to pinpoint it. Once you have this equivalence, it's almost guaranteed that the CI will report a difference of less than 1e-5, since it runs on CPU.
Let me know if you have any questions / queries about finishing this PR. You've done a great job and I'd be more than happy to assist you in seeing this to completion!
|
Ohhhhh I finally pass the equivalence issue! 🎉🎉
But there are CI failures...
|
|
Well done @HeegyuKim, that's excellent news! Regarding the two failing tests:
from transformers import FlaxAutoModelForCausalLM
model = FlaxAutoModelForCausalLM.from_pretrained("EleutherAI/gpt-neox-20b", from_pt=True)And then push the converted Flax weights to the Hub: model.push_to_hub("EleutherAI/gpt-neox-20b", create_pr=True) |
sanchit-gandhi
left a comment
There was a problem hiding this comment.
Looks in great shape @HeegyuKim! And nice job on getting equivalence with PyTorch! Left a few suggestions below, mainly just small re-factoring to get it ready for merge. Feel free to ping me as soon as you're ready for a final look - think it should be pretty fast to get it merged from here
There was a problem hiding this comment.
We've only implemented one of the three possible rotary embedding types (rope_scaling=None). There are two more RoPE types in the PyTorch modelling code:
I don't think it would be too much work to add these so that we have equivalence with the PyTorch modelling code? WDYT?
There was a problem hiding this comment.
I copied RoPE scaling code and test code. But in test CI, the frozen flax model raises SetAttributeFrozenModuleError.
For the cached RoPE embedding, I think I should use the variable. I'm trying to implement it, and I think it'd be nice if you had an appropriate reference or suggestion. @sanchit-gandhi
There was a problem hiding this comment.
Ah we can only set attributes in Flax in the setup method. After that, the module gets frozen, meaning we can't add new attributes or update existing ones. So probably the cached embedding isn't going to work - what we can instead do is always initialise the embeddings to max length (config.max_position_embeddings), and then slice the first N embeddings as required each time. This way, we don't ever need to re-compute or update the embeddings, since we always have the max embedding length we require stored in the setup
There was a problem hiding this comment.
Note to reviewer: we use a causal attention mask in the Flax generation tests. This is required to get sensible outputs from the Flax model (which is typical behaviour).
|
I think we're almost at the end of our work but there are small issues. Suddenly wav2vec2 test fails??
Flax weights
|
|
Hey @HeegyuKim! Nice job on iterating here! Answering your questions in-line below:
Note that it's important you force push ( |
|
Finally documentation is left, how can I make a documentation for it? @sanchit-gandhi |
|
You can do so with |
|
I may passed necessary CI tests! @sanchit-gandhi |
|
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
sanchit-gandhi
left a comment
There was a problem hiding this comment.
Very nice @HeegyuKim! Sorry about the delay with getting you another review. It's mainly small nits from me. Let's put in the request for a core maintainer to take a final look at this and get it merged!
There was a problem hiding this comment.
Thanks for adding this!
There was a problem hiding this comment.
Note to reviewer: single-letter variables chosen to maintain equivalence with the PyTorch modelling code
There was a problem hiding this comment.
The embedding classes are very nice! Ported to JAX while following closely the logic from PyTorch
There was a problem hiding this comment.
Can we not copy this entire method from GPT Neo as well? The code looks to be one-to-one the same now, it's just a comment which is different "# if past_key_values are passed..."
If we do so, then we can actually just copy the entire class from GPT Neo, which would make the copied from statements much simpler.
There was a problem hiding this comment.
What about possibly tied word embeddings?
transformers/src/transformers/models/gpt_neo/modeling_flax_gpt_neo.py
Lines 635 to 637 in 2c658b5
ArthurZucker
left a comment
There was a problem hiding this comment.
Great work here! @HeegyuKim Thanks for adding the flax support 🤗
There was a problem hiding this comment.
| # Copyright 2023 The EleutherAI and The HuggingFace Inc. team. | |
| # Copyright 2023 The HuggingFace Inc. team. |
There was a problem hiding this comment.
I think we got rid of the extra dimensions in the pytorch version #26162
There was a problem hiding this comment.
thanks for the pointers! 😉
There was a problem hiding this comment.
it's tiny bit counter intuitive that the split_head does not split! It's a nit but here's what we have in bloom for the same function:
transformers/src/transformers/models/persimmon/modeling_persimmon.py
Lines 237 to 252 in 587b8e6
There was a problem hiding this comment.
(other jax implementation have the same so feel free to choose what you prefere
There was a problem hiding this comment.
this is only used once so a bit useless, let's remove it
There was a problem hiding this comment.
why do we have to use this? (Just FMI 🤗 )
There was a problem hiding this comment.
We'll have a # Ignore copy soon 😉
|
The PR is almost finished! Would you like to make the last remaining changes @HeegyuKim such that we can get this one merged? Let us know if you have any questions, more than happy to help here |
|
Thank you for your comment! I'll check it this weekend |
This reverts commit a670443b2e4fdd88262c93eb98a9dc0330c64812.
|
Hello @sanchit-gandhi, I rebased this PR to main branch and pushed again. There are two CI failures - First is a documentation issue. This problem can be fixed when this GPT-NeoX model PR is merged. Alternatively, we can add from_pt=True to the example. As for the second issue, I don't know why. I would appreciate it if you could tell me the cause and solution to this problem. I ran |
|
This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread. Please note that issues that do not follow the contributing guidelines are likely to be ignored. |
What does this PR do?
Fixes #22950:
@sanchit-gandhi