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

[L0 provider] log each call to L0 #1068

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

igchor
Copy link
Member

@igchor igchor commented Jan 31, 2025

In a format that is required by SYCL tests.

This is to match UR behavior that logs every call to L0.

Description

Checklist

  • Code compiles without errors locally
  • All tests pass locally
  • CI workflows execute properly
  • CI workflows, not executed per PR (e.g. Nightly), execute properly
  • New tests added, especially if they will fail without my changes
  • Added/extended example(s) to cover this functionality
  • Extended the README/documentation
  • All newly added source files have a license
  • All newly added source files are referenced in CMake files
  • Logger (with debug/info/... messages) is used

Sorry, something went wrong.

@igchor igchor requested a review from a team as a code owner January 31, 2025 17:31
using a special format that SYCL tests rely on
@bratpiorka
Copy link
Contributor

@igchor LGTM, but I think changes made for specific clients should be avoided or we should make it more configurable, but I don't have a simple idea how to do that

@igchor
Copy link
Member Author

igchor commented Feb 3, 2025

@igchor LGTM, but I think changes made for specific clients should be avoided or we should make it more configurable, but I don't have a simple idea how to do that

Yeah, I know this is not ideal. I will be trying to get rid of log parsing logic from SYCL so that we do not have to follow a specific format in future (but this might take some time, until all the tests are fixed).

@vinser52
Copy link
Contributor

vinser52 commented Feb 3, 2025

@igchor LGTM, but I think changes made for specific clients should be avoided or we should make it more configurable, but I don't have a simple idea how to do that

Yeah, I know this is not ideal. I will be trying to get rid of log parsing logic from SYCL so that we do not have to follow a specific format in future (but this might take some time, until all the tests are fixed).

BTW, you are trying to log every Level Zero call. Why not to make such logging on the Level Zero side?

@igchor
Copy link
Member Author

igchor commented Feb 3, 2025

@igchor LGTM, but I think changes made for specific clients should be avoided or we should make it more configurable, but I don't have a simple idea how to do that

Yeah, I know this is not ideal. I will be trying to get rid of log parsing logic from SYCL so that we do not have to follow a specific format in future (but this might take some time, until all the tests are fixed).

BTW, you are trying to log every Level Zero call. Why not to make such logging on the Level Zero side?

Yes, eventually, I would like to have logging inside L0. However, this will require a few more fixes/changes in the loader. For now i just wanted have something that we can use with SYCL right away.

@vinser52
Copy link
Contributor

vinser52 commented Feb 3, 2025

@igchor LGTM, but I think changes made for specific clients should be avoided or we should make it more configurable, but I don't have a simple idea how to do that

Yeah, I know this is not ideal. I will be trying to get rid of log parsing logic from SYCL so that we do not have to follow a specific format in future (but this might take some time, until all the tests are fixed).

BTW, you are trying to log every Level Zero call. Why not to make such logging on the Level Zero side?

Yes, eventually, I would like to have logging inside L0. However, this will require a few more fixes/changes in the loader. For now i just wanted have something that we can use with SYCL right away.

I confused than.
It sounds like you want this PR to be merged as a temporary workaround to fix the SYCL test. is it right? Two things that confuse me:

  1. There is only an issue with the SYCL malformed test.
  2. This PR wants a temporary workaround to be merged into UMF. Furthermore, this changes are client specific as @bratpiorka noticed.

Based on that I have the following questions:

  1. What is the urgency and priority of the issue you are trying to fix?
  2. Why not fix the test if it is malformed?
  3. If such logs eventually should be part of L0, what we will do once they are merged into L0 implementation? The SYCL test will fail again?

@igchor
Copy link
Member Author

igchor commented Feb 3, 2025

The logging is needed not only to make SYCL tests pass but also for debugging/development (and for v2 L0 adapter to have the same capabilities as the legacy adapter).

Let me mark this as draft for now and see if we can add this logging to L0 soon. If not, I will undraft the PR.

@igchor igchor marked this pull request as draft February 3, 2025 17:34
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.

None yet

3 participants