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

$ORT_ALLOW_VERBOSE_LOGGING_ON_RELEASEを導入 #10

Merged

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Dec 7, 2022

VOICEVOX/voicevox_core#338 におけるログにポインタ値等を出せてしまう問題に対し、tracingについてはDEBUG、ORT本体についてはINFO以下の出力を制限することで解決します。
(tracingとORTのログレベルの対応はこんな感じになっているのでややこしいですが)

Copy link
Member Author

@qryxip qryxip left a comment

Choose a reason for hiding this comment

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

補足

Comment on lines -275 to +305
Level::TRACE,
"onnxruntime",
category = category.to_str().unwrap_or("<unknown>"),
file = code_location.file,
line_number = code_location.line_number,
function = code_location.function,
logid = logid.to_str().unwrap_or("<unknown>"),
);
let _enter = span.enter();
let _span = cfg!(allow_verbose_logging).then(|| {
span!(
Level::TRACE,
"onnxruntime",
category = category.to_str().unwrap_or("<unknown>"),
file = code_location.file,
line_number = code_location.line_number,
function = code_location.function,
logid = logid.to_str().unwrap_or("<unknown>"),
)
.entered()
});
Copy link
Member Author

Choose a reason for hiding this comment

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

これについては完全に無害だとは思うが、挙動の一貫性のためこうしておいた。

Copy link
Member Author

@qryxip qryxip Dec 7, 2022

Choose a reason for hiding this comment

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

ここにおける"span"は、このonnxruntime{…}の中。

2022-12-07T14:39:09.569061Z  INFO onnxruntime{category="onnxruntime" file="graph.cc" line_number="3559" function="CleanUnusedInitializersAndNodeArgs" logid=""}: onnxruntime::onnxruntime: "Removing initializer \'664\'. It is not used by any node and should be removed from the model."
2022-12-07T14:39:09.569084Z  INFO onnxruntime{category="onnxruntime" file="graph.cc" line_number="3559" function="CleanUnusedInitializersAndNodeArgs" logid=""}: onnxruntime::onnxruntime: "Removing initializer \'661\'. It is not used by any node and should be removed from the model."
2022-12-07T14:39:09.569096Z  INFO onnxruntime{category="onnxruntime" file="graph.cc" line_number="3559" function="CleanUnusedInitializersAndNodeArgs" logid=""}: onnxruntime::onnxruntime: "Removing initializer \'660\'. It is not used by any node and should be removed from the model."

Copy link
Member Author

@qryxip qryxip Dec 7, 2022

Choose a reason for hiding this comment

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

(あとallow_debug_loggingのつもりでallow_verbose_loggingと書いていたので修正しました)

やっぱりallow_verbose_loggingの方が適切だと思ったのでそうしました。

@@ -145,6 +145,26 @@ macro_rules! extern_system_fn {
($(#[$meta:meta])* $vis:vis unsafe fn $($tt:tt)*) => ($(#[$meta])* $vis unsafe extern "C" fn $($tt)*);
}

// textual scopeで`trace!`と`debug!`を宣言することにより、`tracing`のそれらの`use`を封じる
Copy link
Member Author

@qryxip qryxip Dec 7, 2022

Choose a reason for hiding this comment

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

textual scopeについての解説はこれ。
Textual Scope - Macros By Example - The Rust Reference

私も思い付きで試してみるまで知らなかったが、下のmodやブロックにおける名前解決も邪魔できる。

onnxruntime/build.rs Outdated Show resolved Hide resolved
@qryxip qryxip changed the title $ORT_ALLOW_DEBUG_LOGGINGを導入 $ORT_ALLOW_VERBOSE_LOGGING_ON_RELEASEを導入 Dec 7, 2022
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM!!

DEBUGやINFOレベルはデバッグビルドのときでないと表示させない、という感じだと把握しました!
ありがとうございます!!

Copy link

@qwerty2501 qwerty2501 left a comment

Choose a reason for hiding this comment

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

LGTM

@qwerty2501 qwerty2501 merged commit 405f62f into VOICEVOX:master Dec 28, 2022
@qryxip qryxip mentioned this pull request Jan 20, 2024
12 tasks
qryxip added a commit to qryxip/ort that referenced this pull request Jan 21, 2024
qryxip added a commit to qryxip/ort that referenced this pull request Jan 23, 2024
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