-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use latest apex internal API #8129
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Apex version still has no LayerNorm1p version so can't switch completely
Signed-off-by: Jan Baczek <jbaczek@nvidia.com>
143aab7
to
4d18d26
Compare
@@ -40,7 +40,7 @@ def reset_parameters(self): | |||
torch.nn.init.zeros_(self.bias) | |||
|
|||
def forward(self, x): | |||
return _fast_layer_norm(x, self.weight + 1, self.bias, self.epsilon) | |||
return _fast_layer_norm(x, self.weight + 1, self.bias, self.epsilon, memory_efficient=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this hardcoded to False? Instead it can be run by just passing self.memory_efficient as a parameter right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIU memory_efficient=False is the previous implementation of _fast_layer_norm. As non memory efficient implementation has been tested I left it like this.
We can make it switchable but I think it would be beneficial to test it first. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, we can keep it as it is
Signed-off-by: Jan Baczek <jbaczek@nvidia.com>
Signed-off-by: Jan Baczek <jbaczek@nvidia.com> Signed-off-by: Sasha Meister <ameister@nvidia.com>
Signed-off-by: Jan Baczek <jbaczek@nvidia.com> Signed-off-by: Pablo Garay <pagaray@nvidia.com>
Signed-off-by: Jan Baczek <jbaczek@nvidia.com>
What does this PR do ?
Apex _fast_layer_norm added mandatory argument
memory_efficient
. This PR fixes call to this function to accommodate to the change.Jenkins CI
To run Jenkins, a NeMo User with write access must comment
jenkins
on the PR.Before your PR is "Ready for review"
Pre checks:
PR Type:
Who can review?
@mikolajblaz can you review this one? I can see that you introduced this line 17 months ago. Can we move to the APEX implementation entirely as suggested in the TODO?
Additional Information