Conversation
* Globally enable ms-experimental ops * change meaning of ms_experimental to mean *all* ms_experimental ops. Some experimental ops will still be enabled globally without this flag like audio ops. * add cmath * add cmath to signal_defs.cc * move audio back into experimental, verify on mac * remove experimental from mac builds Co-authored-by: Sheil Kumar <sheilk@microsoft.com>
* fix default initialization value in C API header * Fix conflicts * Nits
…1 wheels (#9101) * make work for both rocm 4.2 and rocm 4.3.1 * fix rocm 4.3.1 docker image reference * fix CUDA_VERSION to ROCM_VERSION * fix ReduceConsts conflict def * add ifdef to miopen_common.h as well * trailing ws
| nullptr}; // TODO: Support arena configuration for users of test runner | ||
| OrtCUDAProviderOptions cuda_options; | ||
| cuda_options.device_id=device_id; | ||
| cuda_options.do_copy_in_default_stream=true; |
There was a problem hiding this comment.
nit: Do we need this line now that it is set to this value by default ? (Same comment for all similar lines in the remaining files)
There was a problem hiding this comment.
I just saw #9064. Have you ever seen the same code generates different behavior in C and C++? Super rarely. Because it is very confusing. We shouldn't add the struct in onnxruntime_c_api.h at first. Then because we fear of making breaking changes, we replicate the error and make it worse and worse. And at the same time, we didn't provide backward compatibility either.
There was a problem hiding this comment.
Agreed. I think it is better to cherry pick Ryan’s change as well..
There was a problem hiding this comment.
I just saw #9064. Have you ever seen the same code generates different behavior in C and C++? Super rarely. Because it is very confusing. We shouldn't add the struct in onnxruntime_c_api.h at first. Then because we fear of making breaking changes, we replicate the error and make it worse and worse. And at the same time, we didn't provide backward compatibility either.
What's the suggest change in this case?
There was a problem hiding this comment.
OrtCUDAProviderOptions is a C struct. It shouldn't have constructors. When needed, we should either add an init function for it, like InitOrtCUDAProviderOptions, or add a c++ wrapper in onnxruntime_cxx_api.h. The c++ wrapper class should have a different name, like Ort::CUDAProviderOptions. Or we do both. Adding a C API is needed, otherwise C# can't use it.
There was a problem hiding this comment.
OrtCUDAProviderOptions is broken currently. It shouldn't have been defined in the C header file to begin with. But we're stuck with it. And yes C# cannot use it. Tensorrt has the same issue. But for tensorrt we introduced a V2 where the struct is not defined in the header; you request an instance from the API. Hence my suggestion is that in the next release we should introduce a similar v2 and deprecate its usage like we do for tensorrt. The addition of _cpluscplus is just a convenience measure, not a solution.
There was a problem hiding this comment.
Our code should demonstrate good usage of onnxruntime C/C++ API. If someone saw this code and copies it to his C source file, then it crashed. He might not know why the same code works here but not there. For example, let's the author in #9136 were using the new API: OrtCUDAProviderOptions. If you just look the code, you won't know if OrtCUDAProviderOptions will be initialized or not unless the author tells you if he saved the code in a file ends with .c or .cpp.
There was a problem hiding this comment.
@snnn are you suggesting that we keep the old way of initializing OrtCUDAProviderOptions in this file? If yes, let's do it and move ahead. Obviously this won't fix the issue of this struct taking on different values in .c and .cc files. Fixing it requires removing the cpluscplus macro introduced in #9064.
There was a problem hiding this comment.
Let's fix this the right way in the next release where we'll deprecate this struct altogether.
Description: Describe your changes.
Motivation and Context