Skip to content
This repository was archived by the owner on Jan 25, 2023. It is now read-only.

Initial support for Numpy subclasses (for dparray) #116

Closed
wants to merge 6 commits into from
Closed

Initial support for Numpy subclasses (for dparray) #116

wants to merge 6 commits into from

Conversation

DrTodd13
Copy link

@DrTodd13 DrTodd13 commented Nov 12, 2020

Copy link

@diptorupd diptorupd left a comment

Choose a reason for hiding this comment

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

@DrTodd13 I looked at the dparray.py implementation. I want some minor code quality issues addressed. Also, can you please add a module level docstring with some example?

@PokhodenkoSA PokhodenkoSA mentioned this pull request Nov 24, 2020
50 tasks
@PokhodenkoSA
Copy link

PokhodenkoSA commented Nov 24, 2020

This PR should be eventually splited into two PRs - one for Numba and another one for numba-dppy.
This PR modifies a lot of Numba core code, introduces dependency numba -> numba-dppy and modifies setup.py.

I could take care of spliting this PR to numba and numba-dppy PRs, and will introduce optional dependency to numba-dppy (like in #118),
but Numba code modifications should be considered in scope of readiness to go to upstream. For some time this modifications could be in IntelPython/numba patches.

I do not know, maybe there is an intention to move dparray to upstream too. Is it?

@DrTodd13
Copy link
Author

@PokhodenkoSA I am going to open a PR today to move the dparray portion into dpctl. The part that interfaces dparray to Numba will remain in dppy. The Numba changes are in merge_subclass PR.

@PokhodenkoSA
Copy link

Related to #75 and IntelPython/dpctl#184.

@PokhodenkoSA
Copy link

PokhodenkoSA commented Nov 30, 2020

This PR will be refactored in following way:

  1. Part of changes will stay in IntelPython/numba. Most of them also contain in PR to upstream Initial support for Numpy subclasses. numba/numba#6148. It relates to Numba processing of nparray subclasses.
  2. Parts of tests will stay in IntelPython/numba-dppy but will transformed to use Python unittest.
  3. Part of tests will move to dpCtl to tests dparray. See Initial separation of dparray into 2 parts. dpctl#184.
  4. dparray_register() activation will move to numba-dppy extension and will be activated via numba extensions mechanics.
  5. Setup.py changes and dppl_rt.c (for USM allocation) will move to numba-dppy extension.

NRT_external_malloc_func malloc;
NRT_external_realloc_func realloc;
NRT_external_free_func free;
void *opaque_data;

Choose a reason for hiding this comment

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

As discussed, the opaque_data needs to be a function pointer to DPCTLQmgr_GetCurrentQueue so that the external allocator always uses the current queue from dpctl for memory allocation. The current implementation stores the queue only once and after that it is never updated, leading to potential issues if the current queue is changed inside dpctl.

Copy link
Author

Choose a reason for hiding this comment

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

The spot to mention this would be in dppl_rt.c where we fill in the opaque value. This here is still fine since opaque_data can also store a function pointer.

@PokhodenkoSA
Copy link

PokhodenkoSA commented Dec 9, 2020

I will rebase this branch on top of patched. As I can not push new history to forked branch I will create a new branch in this repo.
Created #139.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants