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

Adust GetVersionString() GetBuildInfoString() signatures and move them to OrtApi #15921

Merged
merged 5 commits into from
May 13, 2023

Conversation

yuslepukhin
Copy link
Member

@yuslepukhin yuslepukhin commented May 12, 2023

Description

This PR partially reverts changes introduced in #15643

We make two API return std::string always in UTF-8.

We also move the entry points from OrtApiBase to OrtApi to make them versioned.

Motivation and Context

GetVersionString always returns x.y.z numbers that are not subject to internationalization.
GetBuildInfoString can hold international chars, but UTF-8 should be fine to contain those.
We prefix them with u8"" in case the compiler default charset is not UTF-8.
Furthermore, creating platform dependent APIs is discouraged.
ORTCHAR_T is platform dependent and was created for paths only.
On non-unix platforms would still produce std::string that can only contain UTF-8

The API was introduced after the latest release, and can still be adjusted.

pranavsharma
pranavsharma previously approved these changes May 12, 2023
pranavsharma
pranavsharma previously approved these changes May 12, 2023
@@ -629,7 +631,7 @@ struct OrtApiBase {
* older than the version created with this header file.
*/
const OrtApi*(ORT_API_CALL* GetApi)(uint32_t version)NO_EXCEPTION;
const ORTCHAR_T*(ORT_API_CALL* GetVersionString)(void)NO_EXCEPTION; ///< Returns a null terminated string of the version of the Onnxruntime library (eg: "1.8.1")
const char*(ORT_API_CALL* GetVersionString)(void)NO_EXCEPTION; ///< Returns a null terminated string of the version of the Onnxruntime library (eg: "1.8.1")
const ORTCHAR_T*(ORT_API_CALL* GetBuildInfoString)(void)NO_EXCEPTION; ///< Returns a null terminated string of the build info including git info and cxx flags
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be moved into the C API too, as there is no way to detect if it exists. Anyone using a mismatched onnxruntime will just crash.

@snnn
Copy link
Member

snnn commented May 12, 2023

The APIs are not fully implemented yet. They may not work properly with our release packages.

@snnn
Copy link
Member

snnn commented May 12, 2023

ORTCHAR_T is platform dependent and was created for paths only.

I created the macro. But I do not think so. The idea behind of it is: All Windows APIs only use wide chars, we should do the same. Now when we see a std::string in our code, it's hard to tell whether the string is encoded with UTF-8 or a Windows code page. And only we have the confusion. Most Windows applications do not use UTF-8. Because of the confusion, we have a number of such mis-encoding issues reported in GitHub issues. For example, now you change the "GetVersionString" function to return a UTF-8 string, but how many people know they cannot pass the returned value to printf/std::cout if the string contains non-ASCII chars? I suggest adding a comment explicitly saying this function could only return ASCII characters. But, remember, the letter A in ASCII means American. We do not build software only for Americans. So in general we should not use this word.

@yuslepukhin yuslepukhin changed the title Revert GetVersionString() to return UTF-8 information. Adust GetVersionString() GetBuildInfoString() signatures and move them to OrtApi May 12, 2023
@@ -29,7 +29,7 @@ using namespace onnxruntime;

namespace {
void usage() {
auto version_string = ToUTF8String(OrtGetApiBase()->GetVersionString());
auto version_string = Ort::GetVersionString();
Copy link
Member

Choose a reason for hiding this comment

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

For example, here you pass a std::string directly to printf, it works only if the std::string only contains ASCII chars. But most people are not aware of that.

Copy link
Member Author

Choose a reason for hiding this comment

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

We define all strings as UTF-8 internally, except Windows paths. Version string will always be ASCII.

@yuslepukhin
Copy link
Member Author

yuslepukhin commented May 12, 2023

ORTCHAR_T is platform dependent and was created for paths only.

I created the macro. But I do not think so. The idea behind of it is: All Windows APIs only use wide chars, we should do the same. Now when we see a std::string in our code, it's hard to tell whether the string is encoded with UTF-8 or a Windows code page. And only we have the confusion. Most Windows applications do not use UTF-8. Because of the confusion, we have a number of such mis-encoding issues reported in GitHub issues. For example, now you change the "GetVersionString" function to return a UTF-8 string, but how many people know they cannot pass the returned value to printf/std::cout if the string contains non-ASCII chars? I suggest adding a comment explicitly saying this function could only return ASCII characters. But, remember, the letter A in ASCII means American. We do not build software only for Americans. So in general we should not use this word.

I agree. that's why we use UTF-8. We define this as returning a version string x.y.z where there are all numbers. It would not help if those would be in Russian especially in support cases. But last time I checked, Arabic numerals are the same in the languages known to me.

It does not help using ORTTCHAR. UTF-8 is the same on all platforms including Windows. Are you saying that Linux std::cout would display UTF-8 properly? That's what is implied by the current definition.

This appears to clarify it. The problem is not in UTF-8 or std::cout, but in Windows console.

This clarifies it further

Bottom line, we do not want warp the API because of the issues with the OS. But we can document it.

 Make sure both GetBuildInfo() and GetVersionString() are UTF-8 with u8"" prefix.
 Move both entry points from OrtApiBase to OrtApi as they only exist from 1.15 forward.
 Adjust C# and Java
 Pass Environment Creation options by ref.
@@ -629,9 +631,8 @@ struct OrtApiBase {
* older than the version created with this header file.
*/
const OrtApi*(ORT_API_CALL* GetApi)(uint32_t version)NO_EXCEPTION;
const ORTCHAR_T*(ORT_API_CALL* GetVersionString)(void)NO_EXCEPTION; ///< Returns a null terminated string of the version of the Onnxruntime library (eg: "1.8.1")
Copy link
Member

Choose a reason for hiding this comment

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

Oops, I wasn't clear when I mentioned it earlier. The 'GetVersionString' function should stay, it just needs to revert to the original signature:

The structure should look like it does here, before the breaking change:

https://github.com/microsoft/onnxruntime/blob/ebaafac3f5a1e318488a1636686bb9bb911d2db8/include/onnxruntime/core/session/onnxruntime_c_api.h#LL629C1-L629C1

struct OrtApiBase {
  /** \brief Get a pointer to the requested version of the ::OrtApi
   *
   * \param[in] version Must be ::ORT_API_VERSION
   * \return The ::OrtApi for the version requested, nullptr will be returned if this version is unsupported, for example when using a runtime
   *   older than the version created with this header file.
   */
  const OrtApi*(ORT_API_CALL* GetApi)(uint32_t version)NO_EXCEPTION;
  const char*(ORT_API_CALL* GetVersionString)(void)NO_EXCEPTION;  ///< Returns a null terminated string of the version of the Onnxruntime library (eg: "1.8.1")
};

@snnn
Copy link
Member

snnn commented May 12, 2023

For example, if you look the signature of wfopen, even the second parameter could only contain ASCII characters, the type is still a wide-character string. Because it is more consistent. I suggest we follow the same pattern.

@yuslepukhin yuslepukhin merged commit 896a963 into main May 13, 2023
89 checks passed
@yuslepukhin yuslepukhin deleted the yuslepukhin/fix_get_version_string branch May 13, 2023 20:45
prathikr pushed a commit that referenced this pull request May 16, 2023
…m to OrtApi (#15921)

### Description

This PR partially reverts changes introduced in
#15643

We make two API return std::string always in UTF-8.

We also move the entry points from OrtApiBase to OrtApi to make them
versioned.

### Motivation and Context

`GetVersionString` always returns x.y.z numbers that are not subject to
internationalization.
`GetBuildInfoString` can hold international chars, but UTF-8 should be
fine to contain those.
We prefix them with u8"" in case the compiler default charset is not
UTF-8.
Furthermore, creating platform dependent APIs is discouraged.
`ORTCHAR_T` is platform dependent and was created for paths only.
On non-unix platforms would still produce `std::string` that can only
contain UTF-8

The API was introduced after the latest release, and can still be
adjusted.
@snnn snnn added the triage:approved Approved for cherrypicks for release label May 18, 2023
snnn pushed a commit that referenced this pull request May 19, 2023
…m to OrtApi (#15921)

This PR partially reverts changes introduced in
#15643

We make two API return std::string always in UTF-8.

We also move the entry points from OrtApiBase to OrtApi to make them
versioned.

`GetVersionString` always returns x.y.z numbers that are not subject to
internationalization.
`GetBuildInfoString` can hold international chars, but UTF-8 should be
fine to contain those.
We prefix them with u8"" in case the compiler default charset is not
UTF-8.
Furthermore, creating platform dependent APIs is discouraged.
`ORTCHAR_T` is platform dependent and was created for paths only.
On non-unix platforms would still produce `std::string` that can only
contain UTF-8

The API was introduced after the latest release, and can still be
adjusted.
snnn pushed a commit that referenced this pull request May 19, 2023
…m to OrtApi (#15921)

This PR partially reverts changes introduced in
#15643

We make two API return std::string always in UTF-8.

We also move the entry points from OrtApiBase to OrtApi to make them
versioned.

`GetVersionString` always returns x.y.z numbers that are not subject to
internationalization.
`GetBuildInfoString` can hold international chars, but UTF-8 should be
fine to contain those.
We prefix them with u8"" in case the compiler default charset is not
UTF-8.
Furthermore, creating platform dependent APIs is discouraged.
`ORTCHAR_T` is platform dependent and was created for paths only.
On non-unix platforms would still produce `std::string` that can only
contain UTF-8

The API was introduced after the latest release, and can still be
adjusted.
snnn added a commit that referenced this pull request May 19, 2023
### Description
Cherry-picks 26 commits to the release branch. 
Most cherry-picks are clean merges. Except:

1. When I got conflicts in cgmanifest.json and download-deps.yml, I
choose to ignore the conflicts and regenerate the two files
2. There were some conflicts in cmake/deps.txt, onnxruntime_c_api.cc


PR list:

[js/webgpu] fix Transpose with non-float tensor (#15819)
[js/web] fix terser reserved symbols for worker (#15864)
[JSEP] fix constructor for OrtDevice (#15805)
Bump engine.io from 6.4.1 to 6.4.2 in /js/web (#15799)
Bump engine.io from 6.4.0 to 6.4.2 in /onnxruntime/test/wasm (#15798)
[wasm] revert emsdk to v3.1.19 (#15793)
[wasm/JSEP] add threaded build to artifacts (#15777)
[js/web] add target ort.webgpu.min.js (#15780)
update ort extensions to 94142d8391c9791ec71c38336436319a2d4ac7a0 (#15688)
fix: setting builder optimization level to TRT 8.6 default (#15897)
Adust GetVersionString() GetBuildInfoString() signatures and move them to OrtApi (#15921)
Fix segfault for multiple GPU run (regression) (#15823)
android package fix (#15999)
[CoreML EP] Minor changes to allow CoreML EP to handle more nodes and models. (#15993)
Adding support for conv fp16 fusion on Resnet50v1 (#15474)
update onnx release 1.14 for docker files (#15680)
Avoid generating training documentation during packaging (#15795)
Update Conv-Add-Relu Fusion Transformation (#15834)
Fix symbolic shape infer empty value_info (#15842)
NhwcFusedConv: Add before Activation (#15837)
use __hmul2 instead of __hmul2_rn (#15852)
change the EP device to default OrtDevice() for memoryType equals CPU Input (#15903)
Fixing NhwcFusedConv fp16 (#15950)
fix topo sort in quantization tool (#16003)
[doc] add LeakyRelu to coreml supported ops (#15944)
[DML EP] Add frequent upload heap flushing (#15960)

Co-authored-by: Yulong Wang 
Co-authored-by: dependabot[bot] 
Co-authored-by: Guenther Schmuelling 
Co-authored-by: Shalva Mist 
Co-authored-by: Maximilian Müller 
Co-authored-by: Dmitri Smirnov 
Co-authored-by: pengwa 
Co-authored-by: Ashwini Khade 
Co-authored-by: Edward Chen 
Co-authored-by: Jian Chen 
Co-authored-by: liqun Fu 
Co-authored-by: Baiju Meswani 
Co-authored-by: Tianlei Wu 
Co-authored-by: Chen Fu 
Co-authored-by: Ye Wang 
Co-authored-by: cao lei 
Co-authored-by: Yufeng Li 
Co-authored-by: Rachel Guo 
Co-authored-by: Patrice Vignola
@snnn snnn removed triage:approved Approved for cherrypicks for release release:1.15 labels May 19, 2023
preetha-intel pushed a commit to intel/onnxruntime that referenced this pull request Jun 7, 2023
### Description
Cherry-picks 26 commits to the release branch. 
Most cherry-picks are clean merges. Except:

1. When I got conflicts in cgmanifest.json and download-deps.yml, I
choose to ignore the conflicts and regenerate the two files
2. There were some conflicts in cmake/deps.txt, onnxruntime_c_api.cc


PR list:

[js/webgpu] fix Transpose with non-float tensor (microsoft#15819)
[js/web] fix terser reserved symbols for worker (microsoft#15864)
[JSEP] fix constructor for OrtDevice (microsoft#15805)
Bump engine.io from 6.4.1 to 6.4.2 in /js/web (microsoft#15799)
Bump engine.io from 6.4.0 to 6.4.2 in /onnxruntime/test/wasm (microsoft#15798)
[wasm] revert emsdk to v3.1.19 (microsoft#15793)
[wasm/JSEP] add threaded build to artifacts (microsoft#15777)
[js/web] add target ort.webgpu.min.js (microsoft#15780)
update ort extensions to 94142d8391c9791ec71c38336436319a2d4ac7a0 (microsoft#15688)
fix: setting builder optimization level to TRT 8.6 default (microsoft#15897)
Adust GetVersionString() GetBuildInfoString() signatures and move them to OrtApi (microsoft#15921)
Fix segfault for multiple GPU run (regression) (microsoft#15823)
android package fix (microsoft#15999)
[CoreML EP] Minor changes to allow CoreML EP to handle more nodes and models. (microsoft#15993)
Adding support for conv fp16 fusion on Resnet50v1 (microsoft#15474)
update onnx release 1.14 for docker files (microsoft#15680)
Avoid generating training documentation during packaging (microsoft#15795)
Update Conv-Add-Relu Fusion Transformation (microsoft#15834)
Fix symbolic shape infer empty value_info (microsoft#15842)
NhwcFusedConv: Add before Activation (microsoft#15837)
use __hmul2 instead of __hmul2_rn (microsoft#15852)
change the EP device to default OrtDevice() for memoryType equals CPU Input (microsoft#15903)
Fixing NhwcFusedConv fp16 (microsoft#15950)
fix topo sort in quantization tool (microsoft#16003)
[doc] add LeakyRelu to coreml supported ops (microsoft#15944)
[DML EP] Add frequent upload heap flushing (microsoft#15960)

Co-authored-by: Yulong Wang 
Co-authored-by: dependabot[bot] 
Co-authored-by: Guenther Schmuelling 
Co-authored-by: Shalva Mist 
Co-authored-by: Maximilian Müller 
Co-authored-by: Dmitri Smirnov 
Co-authored-by: pengwa 
Co-authored-by: Ashwini Khade 
Co-authored-by: Edward Chen 
Co-authored-by: Jian Chen 
Co-authored-by: liqun Fu 
Co-authored-by: Baiju Meswani 
Co-authored-by: Tianlei Wu 
Co-authored-by: Chen Fu 
Co-authored-by: Ye Wang 
Co-authored-by: cao lei 
Co-authored-by: Yufeng Li 
Co-authored-by: Rachel Guo 
Co-authored-by: Patrice Vignola
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.

None yet

4 participants