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

Refactor native cpu adapter to new logger #1345

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

lplewa
Copy link
Contributor

@lplewa lplewa commented Feb 15, 2024

No description provided.

@lplewa
Copy link
Contributor Author

lplewa commented Feb 20, 2024

I created an octopus merge b45470f of all logs changes (#1359; #1347; #1345; #1342; #1032) and tested integration with intel/llvm repository

@lplewa lplewa changed the title Port native_cpu to new logger framework Refactor native cpu adapter to new logger Feb 20, 2024
@lplewa lplewa marked this pull request as ready for review February 20, 2024 11:25
@lplewa lplewa requested a review from a team as a code owner February 20, 2024 11:25
Copy link
Contributor

@PietroGhg PietroGhg left a comment

Choose a reason for hiding this comment

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

This looks good thank you, only one question: I'm trying to run a simple test built for Native CPU with UR_LOG_LOADER="level:debug;flush:debug;output:stderr" set, what I expect is to see some warnings about functions that are not yet implemented in this adapter, e.g. urPlatformGetBackendOption, but I only get <LOADER>[DEBUG]: Logger loader initialized successfully!, I'm not familiar with the new logger, is there something I'm missing in the env variable?

} \
return UR_RESULT_ERROR_UNSUPPORTED_FEATURE;
do { \
logger::error("Not Implemented : {} - File : {} / Line : ", __FUNCTION__, \
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not very familiar with the syntax for this format string, but isn't this missing a {} for __LINE__ ?

} \
return UR_RESULT_SUCCESS;
do { \
logger::warning("Not Implemented : {} - File : {} / Line : ", \
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, maybe there's a {} missing

@pbalcer
Copy link
Contributor

pbalcer commented Feb 20, 2024

Native CPU with UR_LOG_LOADER="level:debug;flush:debug;output:stderr" set

Each component of UR has its own env variable to control verbosity of logging. This is so that it's possible for the user to see traces for the adapter, without getting spammed with potentially irrelevant messages about the loader.
In this case, you should set UR_LOG_NATIVE_CPU env variable.

@lplewa please add docs here https://github.com/oneapi-src/unified-runtime/blob/main/scripts/core/INTRO.rst#environment-variables (for each adapter you are changing)

@lplewa
Copy link
Contributor Author

lplewa commented Feb 22, 2024

@lplewa please add docs here https://github.com/oneapi-src/unified-runtime/blob/main/scripts/core/INTRO.rst#environment-variables (for each adapter you are changing)

#1376

@lplewa
Copy link
Contributor Author

lplewa commented Mar 14, 2024

@PietroGhg - please review - if you are ok with changes please label it as ready to merge

Copy link
Contributor

@PietroGhg PietroGhg left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@PietroGhg PietroGhg added the ready to merge Added to PR's which are ready to merge label Mar 14, 2024
This commit refactors the native cpu adapter to adopt the new logger
introduced in d9cd223 (Integrate logger with library, 2023-02-03).

Signed-off-by: Łukasz Plewa <lukasz.plewa@intel.com>
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 12.46%. Comparing base (78ef1ca) to head (d05476f).
Report is 177 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1345      +/-   ##
==========================================
- Coverage   14.82%   12.46%   -2.36%     
==========================================
  Files         250      241       -9     
  Lines       36220    36146      -74     
  Branches     4094     4097       +3     
==========================================
- Hits         5369     4506     -863     
- Misses      30800    31636     +836     
+ Partials       51        4      -47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kbenzie kbenzie added the native-cpu Native CPU adapter specific issues label Apr 3, 2024
@kbenzie kbenzie merged commit e2b5b7f into oneapi-src:main Apr 10, 2024
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
native-cpu Native CPU adapter specific issues ready to merge Added to PR's which are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants