Skip to content

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

Merged
merged 11 commits into from
Nov 25, 2021

Conversation

PokhodenkoSA
Copy link
Contributor

@PokhodenkoSA PokhodenkoSA commented Nov 19, 2021

Relates to IntelPython/numba-dpex#630.

  • Reverse dependencies
    • numba-dppy registers context manager
    • dpctl provides common functionality to register nested contexts
  • tests
  • docs
  • changelog
  • dpctl does not know numba-dppy. Now dpctl helps numba-dppy to register by importing it. Use technique like numba plug-ins.

@github-actions
Copy link

@coveralls
Copy link
Collaborator

coveralls commented Nov 19, 2021

Coverage Status

Coverage increased (+0.04%) to 74.826% when pulling 30d340b on spokhode/enh/device_context into 29c2cbc on master.

@oleksandr-pavlyk oleksandr-pavlyk changed the title Enable offloading for numba.njit in dpctl.deveice_context Enable offloading for numba.njit in dpctl.device_context Nov 19, 2021
@@ -111,6 +112,7 @@
"get_current_queue",
"get_num_activated_queues",
"is_in_device_context",
"nested_context_factories",
Copy link
Contributor

@oleksandr-pavlyk oleksandr-pavlyk Nov 23, 2021

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@PokhodenkoSA PokhodenkoSA Nov 24, 2021

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 than dpctl

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) above nested_context_factories.

Agree. As they are located close to each other, so clean up will be easy and in the same time.

Copy link
Contributor Author

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]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PokhodenkoSA
Copy link
Contributor Author

PokhodenkoSA commented Nov 25, 2021

I rebased on release0.11.2 and will merge to release0.11.2 branch.

@PokhodenkoSA PokhodenkoSA merged commit 352d874 into release0.11.2 Nov 25, 2021
@PokhodenkoSA PokhodenkoSA mentioned this pull request Nov 25, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request numba-dppy Requests from numba-dppy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants