Fix mup for the layers with AttentionLayerMup #494
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…as `in_dim` in gps layer
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #494 +/- ##
==========================================
+ Coverage 71.35% 71.52% +0.17%
==========================================
Files 94 93 -1
Lines 8718 8707 -11
==========================================
+ Hits 6221 6228 +7
+ Misses 2497 2479 -18
Flags with carried forward coverage won't be shown. Click here to find out more.
|
DomInvivo
commented
Dec 20, 2023
Comment on lines
+1330
to
+1332
| assert ( | ||
| x[k] % num_heads == 0 | ||
| ), f"embed_dim={x[k]} is not divisible by num_heads={num_heads}" |
There was a problem hiding this comment.
I don't think it's needed since there's another assertion in AttentionLayerMup. @maciej-sypetkowski can you check if we remove that part when scaling by a factor that's not divisible by num_heads, does it still work?
There was a problem hiding this comment.
Yes, it's not needed
maciej-sypetkowski
approved these changes
Dec 24, 2023
env.yml
Outdated
| @@ -17,6 +17,7 @@ dependencies: | |||
| - pandas >=1.0 | |||
| - scikit-learn | |||
| - fastparquet | |||
| - networkx | |||
There was a problem hiding this comment.
Why is it needed now?
DomInvivo
added a commit
that referenced
this pull request
Dec 24, 2023
Removed double check of embed_dim/num_heads, discussed in PR #494
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelogs
embed_dimto the list of keys to look for when doing the mup kwargs@maciej-sypetkowski I think this should fix your issue, although I can't verify it because on my end, when I use the config with
architecture.mup_scale_factor: 2it works. Basically, I can't reproduce because I don't know how you do your scaling for it to fail, but at least theattn_layerkeys inmup_base_params.yamlare no longernull.IMPORTANT
when this PR closes, it will affect the reproducibility of your models if they use
AttentionLayerMup, such asGPSLayerPygsince the mup will affect the learning rate of these layers, whether they were previously ignored.