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.

Sign up for GitHub

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

Jump to bottom

Add bfloat16 to all relevant ops #6079

Closed
wants to merge 4 commits into from
Closed

Add bfloat16 to all relevant ops #6079

wants to merge 4 commits into from

Conversation

thiagocrepaldi
Copy link
Contributor

@thiagocrepaldi thiagocrepaldi commented Apr 11, 2024 •
edited by justinchuby

Fixes #3842
Fixes #6071
Fixes #4051

@justinchuby
Copy link
Contributor

Much needed improvement, thanks!

@justinchuby justinchuby added the operator Issues related to ONNX operators label Apr 11, 2024
@thiagocrepaldi thiagocrepaldi marked this pull request as ready for review April 11, 2024 21:33
@thiagocrepaldi thiagocrepaldi requested review from a team as code owners April 11, 2024 21:33
@thiagocrepaldi

This comment was marked as resolved.

Sign in to view
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 11, 2024
View reviewed changes
onnx/defs/math/old.cc
@@ -11,6 +11,978 @@

namespace ONNX_NAMESPACE {

bool BuildContextDependentFunctionBody_opset13(

Check warning

Code scanning / CodeQL

Poorly documented large function Warning

Poorly documented function: fewer than 2% comments for a function of 111 lines.
onnx/defs/nn/old.cc
return {"tensor(float16)", "tensor(float)", "tensor(double)"};
}

std::function<void(OpSchema&)> PoolOpSchemaGenerator_opset19(

Check warning

Code scanning / CodeQL

Poorly documented large function Warning

Poorly documented function: fewer than 2% comments for a function of 113 lines.
onnx/defs/nn/old.cc

ONNX_OPERATOR_SET_SCHEMA(ConvTranspose, 11, OpSchema().FillUsing(ConvTransposeOpSchemaGenerator_opset11("a filter")));

std::function<void(OpSchema&)> ConvOpSchemaGenerator_opset11(const char* filter_desc) {

Check warning

Code scanning / CodeQL

Poorly documented large function Warning

Poorly documented function: fewer than 2% comments for a function of 103 lines.
onnx/defs/nn/old.cc
convPoolShapeInference_opset19(ctx, true, false, 0, 1);
}));

std::function<void(OpSchema&)> ConvTransposeOpSchemaGenerator_opset11(const char* filter_desc) {

Check warning

Code scanning / CodeQL

Poorly documented large function Warning

Poorly documented function: fewer than 2% comments for a function of 121 lines.
@justinchuby justinchuby added the auto update doc Generate md/proto files automatically using the CI pipeline label Apr 11, 2024
@justinchuby justinchuby reopened this Apr 11, 2024
@justinchuby
Copy link
Contributor

Just added the doc update tag that will handle it for you. It will generate the files in CI.

@thiagocrepaldi thiagocrepaldi marked this pull request as draft April 11, 2024 23:08
@thiagocrepaldi
Copy link
Contributor Author

folks, my repo was outdated. hold on the review. need to add a few more ops

@thiagocrepaldi
Copy link
Contributor Author

Just added the doc update tag that will handle it for you. It will generate the files in CI.

how to do that on my own for next time?

@justinchuby
Copy link
Contributor

justinchuby commented Apr 11, 2024 via email

@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/fix-3842 branch 2 times, most recently from bc246c8 to c1ccb9c Compare April 12, 2024 14:39
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 12, 2024
View reviewed changes
onnx/defs/rnn/old.cc
}
}
}
std::function<void(OpSchema&)> RNNDocGenerator_opset14(const char* /*name*/) {

Check warning

Code scanning / CodeQL

Poorly documented large function Warning

Poorly documented function: fewer than 2% comments for a function of 103 lines.
@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/fix-3842 branch 2 times, most recently from eaaa268 to 5d1159f Compare April 12, 2024 15:19
@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/fix-3842 branch 5 times, most recently from f24c150 to 25471b8 Compare April 12, 2024 16:32
@thiagocrepaldi thiagocrepaldi marked this pull request as ready for review April 12, 2024 16:43
@thiagocrepaldi thiagocrepaldi mentioned this pull request Apr 23, 2024
Draft
@thiagocrepaldi
Copy link
Contributor Author

Reference

The issue only happens when I create defs/training/old.cc and move Gradient-1 to it. Reverted this piece as it seems that only creating old.cc is not sufficient

#6097

@thiagocrepaldi thiagocrepaldi removed the auto update doc Generate md/proto files automatically using the CI pipeline label Apr 23, 2024
cbourjau
cbourjau reviewed Apr 24, 2024
View reviewed changes
docs/Changelog-ml.md Outdated Show resolved Hide resolved
@thiagocrepaldi
Add bfloat16 to all relevant ops
6c6d418 
Signed-off-by: Thiago Crepaldi <thiagofc@microsoft.com>
@thiagocrepaldi
Manual backend data?
cf5ab8f 
Signed-off-by: Thiago Crepaldi <thiagofc@microsoft.com>
gramalingam
gramalingam reviewed Apr 24, 2024
View reviewed changes
onnx/test/test_backend_onnxruntime.py
@@ -185,6 +185,272 @@ def run_node(cls, node, inputs, device=None, outputs_info=None, **kwargs):
"|test_transpose_"
"|test_unsqueeze_"
"|test_wrap_pad_"
"|test_acos_cpu"
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can add support for excluding models based on IR version and opset-version? It would be much simpler to say backend_test.exclude_ir_version(lambda irversion: irversion > 9)

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the code, I guess we should add such a function here. And use it here. I am not quite sure how to check for the ir-version. I see that TestItem has a proto field which we must use, I guess, but not quite sure I understand what is in this field.

@thiagocrepaldi
Delete traditional ml ops
67c2f63 
Signed-off-by: Thiago Crepaldi <thiagofc@microsoft.com>
@thiagocrepaldi thiagocrepaldi added the auto update doc Generate md/proto files automatically using the CI pipeline label Apr 24, 2024
@thiagocrepaldi @github-actions
CI:apply auto updated documentation/backend test data
bbcafba 
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@thiagocrepaldi thiagocrepaldi removed the auto update doc Generate md/proto files automatically using the CI pipeline label Apr 24, 2024
@thiagocrepaldi thiagocrepaldi mentioned this pull request Apr 24, 2024
Merged
@thiagocrepaldi
Copy link
Contributor Author

#6099 was created because I cant reopen this PR for some reason

github-merge-queue bot pushed a commit that referenced this pull request Apr 25, 2024
@thiagocrepaldi
Add bfloat16 to all relevant ops (#6099)
4e7289d 
Reland #6079

Fixes #3842
Fixes #6071
Fixes #4051

@gramalingam @justinchuby FYI

Signed-off-by: Thiago Crepaldi <thiagofc@microsoft.com>
isdanni pushed a commit to isdanni/onnx that referenced this pull request May 6, 2024
@thiagocrepaldi @isdanni
Add bfloat16 to all relevant ops (onnx#6099)
c7b80ec 
Reland onnx#6079

Fixes onnx#3842
Fixes onnx#6071
Fixes onnx#4051

@gramalingam @justinchuby FYI

Signed-off-by: Thiago Crepaldi <thiagofc@microsoft.com>
Signed-off-by: isdanni <leedanni@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbourjau cbourjau cbourjau left review comments

@gramalingam gramalingam gramalingam left review comments

@justinchuby justinchuby justinchuby approved these changes

Assignees

@thiagocrepaldi thiagocrepaldi

Labels
operator Issues related to ONNX operators release notes Important changes to call out in release notes run release CIs Use this label to trigger release tests in CI
Projects
PR Tracker
Status: Done
Milestone
1.17
Development

Successfully merging this pull request may close these issues.

[Feature request] Bump Conv to accept bfloat16 conv bfloat16 Add bfloat16 to all relevant ops
4 participants
@thiagocrepaldi @justinchuby @gramalingam @cbourjau