Skip to content

Implement loader config object, allow programatic layer management. #681

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 4 commits into from
Jul 18, 2023

Conversation

aarongreig
Copy link
Contributor

No description provided.

@aarongreig
Copy link
Contributor Author

this could do with some dedicated unit testing but I'm not really sure where, would feel out of place in the CTS since no adapter code will ever get called

@@ -28,13 +28,14 @@ context_t::context_t() {
xptiFrameworkInitialize();

call_stream_id = xptiRegisterStream(CALL_STREAM_NAME);
traceEnvEnabled = getenv_tobool("UR_ENABLE_TRACING");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a rock-and-a-hard-place type situation. There is already an XPTI env var that you need to set to enable the tracing used by this layer (actually there are several) - adding this new env var is our rock (detracts from user experience, seems generally redundant).
So the obvious thing would be to treat the XPTI one as our layer-enabling env var instead of adding the new one - but if we do that the API mechanism becomes redundant. Enabling the XPTI variable is always needed for the layer to work - you'd need to set it even if you wanted to enable your layers via api. This is our hard place, this solution makes the API redundant, and might lead to users making unnecessary API calls. At best it would be weird to document.
There is also a third, un-named, unpleasant location: we could just not report this tracing layer through the api mechanism and leave it as a kind of power-user option. This would preserve the current behaviour for existing users without being potentially misleading by reporting its existence through the loader config mechanism. That kind of sucks though because we have this functionality so we should encourage people to use it. It would also feel weird to have a layer that's fundamentally inconsistent with the layer mechanism we're trying to set up.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had the same dilemma. I imagine most users will leverage this layer indirectly through tools like urtrace or just sycl, so the ergonomics of this are not that important. It's already complicated and requires at least some understanding of the XPTI framework.
So I'm fine with adding the new env variable, provided it's well documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, added some documentation of the new env var mechanism and all the layers in INTRO.rst

@veselypeta
Copy link
Contributor

this could do with some dedicated unit testing but I'm not really sure where, would feel out of place in the CTS since no adapter code will ever get called

There are dedicated loader tests under test/loader which would seem reasonable?

@pbalcer
Copy link
Contributor

pbalcer commented Jul 4, 2023

this could do with some dedicated unit testing but I'm not really sure where, would feel out of place in the CTS since no adapter code will ever get called

There are dedicated loader tests under test/loader which would seem reasonable?

Yup. I'd recommend just creating a new folder in that location for the config. These tests are less structured than the CTS ones because of the larger variety of test scenarios (e.g., launching the same process multiple times with different environment variables, with expecting predefined output), so just do whatever you think makes the most sense, either normal gtest scenarios or tests that rely on the match mechanism (like most of the existing layer tests).

@aarongreig
Copy link
Contributor Author

cool, will add something in there

@aarongreig aarongreig force-pushed the aaron/layerConfig branch 2 times, most recently from 26f0ab5 to 50a5be2 Compare July 4, 2023 16:11
@aarongreig aarongreig force-pushed the aaron/layerConfig branch from 50a5be2 to c2d037a Compare July 6, 2023 09:43
@aarongreig
Copy link
Contributor Author

Added a CTS style unit test suite in test/loader/loader_config/ (and it immediately caught a bug!)

@aarongreig aarongreig force-pushed the aaron/layerConfig branch 2 times, most recently from d4c1409 to 016b3c3 Compare July 6, 2023 10:14
Copy link
Contributor

@kbenzie kbenzie left a comment

Choose a reason for hiding this comment

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

Couple minor comments

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants