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

Bump various ops to opset 22, adding bf16 support #3059

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

Conversation

jorickert
Copy link
Collaborator

Bumped ops:
Acos
Acosh
Asin
Asinh
Atan
Atanh
AveragePool
Bernoulli
Conv
ConvTranspose
Cos
Cosh
DeformConv
Det
Dropout
Elu
EyeLike
GRU
GlobalAveragePool
GlobalMaxPool
HardSigmoid
HardSwish
InstanceNormalization
LSTM
LpNormalization
LpPool
MaxPool
MaxRoiPool
MaxUnpool
Mish
Multinomial
NegativeLogLikelihoodLoss
RNN
RandomNormal
RandomNormalLike
RandomUniform
RandomUniformLike
RoiAlign
Round
Selu
Sin
Sinh
Softplus
Softsign
Tan
ThresholdedRelu

Bumped ops:
Acos
Acosh
Asin
Asinh
Atan
Atanh
AveragePool
Bernoulli
Conv
ConvTranspose
Cos
Cosh
DeformConv
Det
Dropout
Elu
EyeLike
GRU
GlobalAveragePool
GlobalMaxPool
HardSigmoid
HardSwish
InstanceNormalization
LSTM
LpNormalization
LpPool
MaxPool
MaxRoiPool
MaxUnpool
Mish
Multinomial
NegativeLogLikelihoodLoss
RNN
RandomNormal
RandomNormalLike
RandomUniform
RandomUniformLike
RoiAlign
Round
Selu
Sin
Sinh
Softplus
Softsign
Tan
ThresholdedRelu

Signed-off-by: Rickert, Jonas <[email protected]>
@hamptonm1
Copy link
Collaborator

hamptonm1 commented Jan 30, 2025

@jorickert maybe there is some confusion on how to update these files but what was done is incorrect. We would not simply replace the opset version with the latest one although support is given for it. How the files would be update would be shown below in this example:

"Clip": [13, 12, 11, 6],

Here for Clip we specify the minimum opset version we support along with any of the newer ones. So if "AveragePool": [19] uses bf16 support then the update would be "AveragePool": [22, 19] which means we still keep the minimum version of support.

We might need to check that the operations also support version 20 and 21 if there was like a data type change.

@jorickert
Copy link
Collaborator Author

jorickert commented Jan 30, 2025

Here for Clip we specify the minimum opset version we support along with any of the newer ones.

This is different from how I thought the versioning works. Do really all versions that are supported need to be listed, or only if there is a breaking change between them?
My understanding was that only versions with breaking changes need to be listed.
So for example if SomeOpV21 is a breaking change compared to SomeOpV20, we would need to list [21, 20].
But if SomeOpV22 is a strict superset of SomeOpV21 I thought we can just update the version, without introducing a new copy of the operation, so the version set would be [22, 20]. Is that not correct?

All the changes in this PR are not braking and only extend the type support

@jorickert
Copy link
Collaborator Author

I tested it, and with this PR I can successful parse both Acos Opset 7 as well as Acos Opset 22. I will add a test for this

@philass
Copy link
Member

philass commented Jan 30, 2025

The way @jorickert implemented this is correct. Because opset 22 only adds a new type the ops are backwards compatible. This LGTM

@hamptonm1
Copy link
Collaborator

hamptonm1 commented Jan 30, 2025

@jorickert I would default to @AlexandreEichenberger or @chentong319. Regardless if the ops are backwards compatible, I want to make sure the precedent is followed by the maintainers of the community.

FYI-@philass

@hamptonm1 hamptonm1 self-requested a review January 30, 2025 20:31
Copy link
Collaborator

@hamptonm1 hamptonm1 left a comment

Choose a reason for hiding this comment

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

I am blocking until the maintainers review :)

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.

3 participants