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

Low precision accumulation flag #1958

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nikalra
Copy link
Contributor

@nikalra nikalra commented Aug 25, 2023

  • Adds support for setting MLModelConfiguration.allowLowPrecisionAccumulationOnGPU

Copy link
Collaborator

@TobyRoseman TobyRoseman left a comment

Choose a reason for hiding this comment

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

This is a great feature! Thanks for the pull request.

The code itself look good, but there are a few things that I think should be addressed:

1 - I think we need to be explicit (verbose even) in the naming of this flag in our public APIs. Let call it allow_low_precision_accumulation_on_GPU.

2 - I’d like to see new unit tests created for this functionality. Rather than tacking it on to existing unit tests. Ideally these tests would load the model normally and load model using this flag. Then verify that the results are close.

3 - I think if a user sets this option to True but is also using a compute unit that doesn’t allow GPU, then they should get a warning.

I have kicked off a CI run to this change.

@TobyRoseman TobyRoseman self-assigned this Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants