Skip to content

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