Skip to content
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

Add tests for ck mha fp16 solver #3304

Merged

Conversation

BrianHarrisonAMD
Copy link
Collaborator

Purpose of review:

Note:

  • Merging these changes into the integration branch to allow for limited diff, but will follow up with a PR for merging the integration branch into develop once complete.
  • Testing and solver are limited to FWD inference for now.

@BrianHarrisonAMD
Copy link
Collaborator Author

BrianHarrisonAMD commented Oct 8, 2024

Note #3302 updates requirement.txt CK hash to the version needed for the integration branch.
Ill wait until #3302 is merged to develop before merging the integration branch to develop in order to align these properly (it'll probably be merged first anyways, but mentioning just in case).

Comment on lines +337 to +342
if(m_bernulliProbability > 0.0f)
{
// Due to GPU version using a different dropout generator we will compare to CPU without
// dropout and verify that dropout causes a large difference when comparing results.
EXPECT_GT(oError, errorThreshold);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's only check which does make some sense with the current approach, but the ideal case is specifically prepared data which will allow us to check statistical properties of the dropout - tensor V must have identity matrixes, so it won't affect the data after dropout and we can say that approximately half of the elements are zero for m_bernulliProbability = 0.5.

It's just a side note of how it can be actually checked.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting idea.
CK also allows us to capture the dropout data.
I was thinking it would make sense to capture, and use that for verification, but I figured we could add that as part of enabling the additional optional params.

@junliume junliume merged commit c4bbe21 into bharriso/integrate-ck-mha-fp16 Oct 10, 2024
140 checks passed
@junliume junliume deleted the bharriso/add-tests-ck-mha-fp16 branch October 10, 2024 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants