-
Notifications
You must be signed in to change notification settings - Fork 30
Extending dpctl.device_context
with nested contexts
#678
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
Conversation
View rendered docs @ https://intelpython.github.io/dpctl/pulls/678/index.html |
numba.njit
in dpctl.deveice_context
numba.njit
in dpctl.device_context
@@ -111,6 +112,7 @@ | |||
"get_current_queue", | |||
"get_num_activated_queues", | |||
"is_in_device_context", | |||
"nested_context_factories", |
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 do not see why this needs to be exposed, especially since device_context
is on its way out.
If it were to stay we should need to design an API to manage these factories.
I.e. hold then in a dictionary and provide functions to retrieve the list of keys for the registered factories, to add a factory to this list with some key, to remove a factory by its key.
Even then, these function would belong to dpctl.utils
namespace, rather than dpctl
.
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.
@PokhodenkoSA If we can avoid adding the function here, let us do it. It just adds more clean up for future.
@oleksandr-pavlyk Given that the changes need to make it to 0.12, I am willing to let it slide. Once, device_context
is gone a whole lot of things will have to be deprecated and removed. Basically, all the functions (get_current_queue
and family) above nested_context_factories
.
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.e. hold then in a dictionary and provide functions to retrieve the list of keys for the registered factories, to add a factory to this list with some key, to remove a factory by its key.
The factory is a key. So we can use list. Also I think we should not guaranty order of contexts.
Agree about functions. But I do not want to make API, especially when users will only append nested context once and will not remove it at all - that is the main use case.
belong to
dpctl.utils
namespace, rather thandpctl
Partially agree that this is utils. But I think this should be close to device_context
namespace. Ideally: dpctl.device_context.add_nested_context_factory(factory)
.
Once,
device_context
is gone a whole lot of things will have to be deprecated and removed.
Separate discussion needed: How to make numba redirect context active for compute follows data?
Basically, all the functions (
get_current_queue
and family) abovenested_context_factories
.
Agree. As they are located close to each other, so clean up will be easy and in the same time.
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.
If we can avoid adding the function here, let us do it.
I think list is simple enough. @oleksandr-pavlyk let me keep things as is. I know that API should be good isolation. But in this case I would like to be simple.
@@ -4,10 +4,11 @@ All notable changes to this project will be documented in this file. | |||
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), | |||
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). | |||
|
|||
## [Next Release] - TBD | |||
## [Unreleased] |
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.
See Keep a Changelog
I rebased on release0.11.2 and will merge to release0.11.2 branch. |
Relates to IntelPython/numba-dpex#630.