-
Notifications
You must be signed in to change notification settings - Fork 795
[SYCL][UR] Log sycl-ls adapter errors in verbose mode #17651
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
[SYCL][UR] Log sycl-ls adapter errors in verbose mode #17651
Conversation
|
Looks great! Although I think we should also still downgrade the message from error to info, since there's plenty of cases where it's not an error |
sycl/tools/sycl-ls/sycl-ls.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nullptr is preferred over NULL in C++.
sycl/tools/sycl-ls/sycl-ls.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this not be orig_ur_log_loader_var_str = std::getenv("UR_LOG_LOADER")?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
orig_ur_log_loader_var = std::getenv("UR_LOG_LOADER") (the char* not the string)
sycl/tools/sycl-ls/sycl-ls.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we setting this unconditionally? Someone might want to run UR_LOG_LOADER="level:error;output:file,some_file" sycl-ls --verbose in order to capture the output into a file rather than stderr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I see - what do you suggest in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could parse the variable if it's already set by the user, and:
- set level to error if not already set to a lower level (e.g. user sets
level:info) - set output to stderr if not set already and level is error (I wouldn't redirect info and debug to stderr)
Otherwise, if the variable is unset, then set "level:error;output:stderr".
Pseudocode:
if var_is_set:
if not level_is_set:
level = error
if level >= error and not output_is_set:
output = stderr
else:
level = error
output = stderr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if it is set at all then we should just ignore it; presumably the user knows what they are doing. But Rafbeils' solution is fine by me as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafbiels , I think parsing the log variable might be overly complicated. I agree with @RossBrunton that if the user is already aware of the UR_LOG_LOADER env var and knows how to set it, no further help is needed.
I have updated my PR so that if the UR_LOG_LOADER evn var is set, we do not modify it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, I'm also happy with that 👍
d1aca1e to
f5b5818
Compare
f5b5818 to
6b380ee
Compare
HPS-1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
@intel/llvm-gatekeepers please merge |
|
@llvm-reviewers-runtime please review |
6b380ee to
b8a6485
Compare
|
rebased, |
CI isn't ready yet. Please ping us once it is. |
b8a6485 to
e16a754
Compare
|
I see some errors not related to my changes - are the machines working correctly? default_selector() : No device of requested type available. Error: The action has timed out. @intel/llvm-gatekeepers please merge |
|
im investigating |
Log sycl-ls adapter errors in verbose mode. Additionally, inform the user about verbose mode when no platforms are found.