Update ck_fused_attn logging to direct to thread-specific files#453
Update ck_fused_attn logging to direct to thread-specific files#453
Conversation
Micky774
requested review from
ipanfilo,
wangye805 and
wenchenvincent
as code owners
| @@ -531,7 +541,7 @@ hipError_t ck_attn_bwd( | |||
| mask_enum mask_type = static_cast<mask_enum>(attn_mask_type); | |||
| bool ck_fused_attn_log_config = false; | |||
| if (const char* env_p = std::getenv("CK_FUSED_ATTN_LOG_CONFIG") ) { | |||
There was a problem hiding this comment.
Calling open_ck_fused_attn_log_file() here and using log_file != nullptr will hell avoiding interpreting env variable here and will also enable logging only if ENV variable is set to non "0"
There was a problem hiding this comment.
Done in a variation
|
|
||
| uint64_t get_runtime_max_seqlen(uint64_t b, const void* cu_seqlen_ptr, const void* cu_seqlen_padded_ptr, void* workspace, hipStream_t stream); | ||
|
|
||
| bool open_ck_fused_attn_log_file(std::ofstream& log_file, const char* file_prefix); |
There was a problem hiding this comment.
The result of the method is not used.
| return false; | ||
| } | ||
| const std::string log_dir_str(env_p); | ||
| if (log_dir_str.empty() || log_dir_str == "0") { |
There was a problem hiding this comment.
Please leave option to fallback to stdout. Since "0" is used as DISABLE, "1" can be use for this.
Or if use any non empty string to ENABLE, "-" may indicate stdout
There was a problem hiding this comment.
Updated so that std::cout is used whenever the log-file creation fails, or when the env variable is set to "1" explicitly.
| std::ostream* get_fwd_log_stream() { | ||
| thread_local std::ofstream log_file; | ||
| thread_local bool attempted = false; | ||
| thread_local bool opened = false; |
There was a problem hiding this comment.
Why so many thread_local variables are needed? log_dir_str is not used by further calls at all. requested duplicates log_file.is_open()
There was a problem hiding this comment.
I've streamlined the implementation a bit now
|
|
||
| #include<iostream> | ||
| #include<cstdint> | ||
| #include<cstdlib> |
There was a problem hiding this comment.
Looks like only iostream is needed
| std::string log_dir_str(env_p); | ||
| if (!log_dir_str.empty() && log_dir_str != "0") { | ||
| if (log_dir_str == "1") { | ||
| log_stream = static_cast<std::ostream*>(&std::cout); |
There was a problem hiding this comment.
Static_cast should not be needed here and when assigning log_file too
|
|
||
| #include<iostream> | ||
| #include<cstdint> | ||
| #include<fstream> |
There was a problem hiding this comment.
why does it need fstream here?
There was a problem hiding this comment.
All the ck_fused_attn_*.cpp files require it, so just to factor it out into the common header.
3 participants
Description
Fixes https://github.com/ROCm/frameworks-internal/issues/15080
The main caveat here is that the kernel-level logging provided by AITER/CK is still pointing to
std::coutand we can't redirect it since redirection is process-wide, not thread-local.Implements logging using cached
thread_localstreams to minimize cost, and guarantee thread safety.Type of change
Changes
Checklist: