Add getter/setter of C# OrtEnv log level by yf711 · Pull Request #13402 · microsoft/onnxruntime

Yifan Li (yf711) · 2022-10-21T17:57:33Z

Description

Set C# OrtEnv log level to be a property (getter/setter equipped)
Add C/C++ API about updating ort env with custom log level to support the setter above (Following pybind implementation)
Add test case to verify logLevel property's getter & setter
Fill missing C apis to C# apilist (this is mandatory if those missing apis are earlier than yours)

Motivation and Context

For C++/Python, the log level can be adjusted via OrtEnv, and this feature is missing in C# binding

Pranav Sharma (pranavsharma) · 2022-10-22T01:56:59Z

+   *
+   * \since Version 1.13.
+   */
+  ORT_API2_STATUS(UpdateEnvWithCustomLogLevel, OrtLoggingLevel log_severity_level, _In_ const OrtEnv* ort_env);


You can't really have OrtEnv passed as const when you've an API to update it :-).

Thanks for pointing out! That has been fixed

Please, add C++ API so users do not have to go to C back and forth.

C++ API has been added in latest commit as well. Thanks!

One important thing to mention is thread-safety. Typically, we do not update the environment after the creation and any thread or even multiple sessions can use it.

One important thing to mention is thread-safety. Typically, we do not update the environment after the creation and any thread or even multiple sessions can use it.

Really appreciate the context you shared. The reason adding this env_update feature is this feature exists in Pybind but missing in c# binding, and we would like to unify the user experience and provide the ability to adjust default logger

This feature reuses the existing design in Pybind:set_default_logger_severity. Do you have any suggestions in order to improve this design?

I understand. One thing to note, we usually pass the object ptr as a first argument, as you can see in other examples. Please, adjust.

Chi Lo (chilo-ms) · 2022-10-24T17:06:00Z

        public IntPtr ReleaseKernelInfo;

        public IntPtr GetTrainingApi;
+        public IntPtr SessionOptionsAppendExecutionProvider_CANN;


Are these CANN related declarations expected to be here?

Yes, as all newly added C APIs before mine would also need to be added here, so that the same order could be kept.

I initially learned from this previous commit to add additional APIs to keep order same as C APIs, and this was also mentioned in the code here

Dmitri Smirnov (yuslepukhin) · 2022-10-28T18:15:04Z

+        public LogLevel GetLogLevel()
+        {
+            return currentLogLevel;
+        }


If this is really needed, it should be a property. And you can then combine set/get #Resolved

Thanks for sharing! This has been improved in the latest commit and verified in test case.

Dmitri Smirnov (yuslepukhin)

🕐

Dmitri Smirnov (yuslepukhin) · 2022-11-04T16:49:08Z

+  API_IMPL_BEGIN
+  LoggingManager* default_logging_manager = ort_env->GetLoggingManager();
+  int severity_level = static_cast<int>(log_severity_level);
+  default_logging_manager->SetDefaultLoggerSeverity(static_cast<logging::Severity>(severity_level));


default_logging_manager->SetDefaultLoggerSeverity(static_castlogging::Severity(severity_level));

Need to look at the thread-safety here #Resolved

Dmitri Smirnov (yuslepukhin)

### Description * Add getter/setter to access and update C# OrtEnv log level * Add C API about updating ort env with custom log level to support the setter above (Following [pybind implementation](https://github.com/microsoft/onnxruntime/blob/952c99304a764328070ac6c07f510deec06dedf4/onnxruntime/python/onnxruntime_pybind_state.cc#L923-L924)) * Add test case to verify getter & setter ### Motivation and Context * For C++/Python, the log level can be adjusted via OrtEnv, and this feature is missing in C# binding

Yifan Li (yf711) added 2 commits October 20, 2022 15:03

add getter & setter of c# log level and C api to update ortenv log level

b7796e5

Update C# OrtApi list with recently created C APIs

dadefce

Yifan Li (yf711) marked this pull request as ready for review October 21, 2022 20:31

Yifan Li (yf711) requested a review from Chi Lo (chilo-ms) October 21, 2022 20:34

Pranav Sharma (pranavsharma) reviewed Oct 22, 2022

View reviewed changes

Yifan Li (yf711) and others added 2 commits October 23, 2022 22:54

Fix merge conflict, set OrtEnv not as const, API version correction

e1ef800

Merge branch 'main' into test_c#_log

234e755

Chi Lo (chilo-ms) reviewed Oct 24, 2022

View reviewed changes

Dmitri Smirnov (yuslepukhin) reviewed Oct 28, 2022

View reviewed changes

Dmitri Smirnov (yuslepukhin) suggested changes Oct 28, 2022

View reviewed changes

add c++ api wrapper, set logLevel as property

9715a16

Yifan Li (yf711) requested a review from Dmitri Smirnov (yuslepukhin) November 4, 2022 16:41

Dmitri Smirnov (yuslepukhin) reviewed Nov 4, 2022

View reviewed changes

reorder obj ptr as the first arg

f8f1321

Dmitri Smirnov (yuslepukhin) approved these changes Nov 4, 2022

View reviewed changes

Yifan Li (yf711) merged commit 8b9065a into main Nov 5, 2022

Yifan Li (yf711) deleted the test_c#_log branch November 5, 2022 04:46

Conversation

Yifan Li (yf711) commented Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Uh oh!

Pranav Sharma (pranavsharma) Oct 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dmitri Smirnov (yuslepukhin) Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dmitri Smirnov (yuslepukhin) left a comment

Choose a reason for hiding this comment

Uh oh!

Dmitri Smirnov (yuslepukhin) Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dmitri Smirnov (yuslepukhin) left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Yifan Li (yf711) commented Oct 21, 2022 •

edited

Loading

Pranav Sharma (pranavsharma) Oct 22, 2022 •

edited

Loading

Dmitri Smirnov (yuslepukhin) Oct 28, 2022 •

edited

Loading

Dmitri Smirnov (yuslepukhin) Nov 4, 2022 •

edited

Loading