Skip to content

Fix: Issue #8923 - avoid spurious device-to-host transfers in CUDA ufuncs - #9005

Merged
esc merged 3 commits into
numba:mainfrom
gmarkall:issue-8923
Jun 7, 2023
Merged

esc merged 3 commits into
numba:mainfrom
gmarkall:issue-8923

Conversation

@gmarkall

@gmarkall gmarkall commented Jun 6, 2023

Copy link
Copy Markdown
Member

Only convert an object to a NumPy array when we can't work out the dtype, so that we don't end up converting all arguments to NumPy arrays on the host.

Fixes Issue #8923.

gmarkall added 2 commits June 6, 2023 16:46
This checks that no transfers are incurred by the execution of a ufunc
on device data.
Only convert an object to a NumPy array when we can't work out the
dtype, so that we don't end up converting all arguments to NumPy arrays
on the host.

Fixes Issue numba#8923.
@gmarkall

gmarkall commented Jun 6, 2023

Copy link
Copy Markdown
Member Author

gpuci run tests

@gmarkall gmarkall added this to the Numba 0.57.1-rc1 milestone Jun 6, 2023
@gmarkall
gmarkall marked this pull request as ready for review June 6, 2023 16:28
@esc esc added the Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm label Jun 6, 2023
@esc

esc commented Jun 6, 2023

Copy link
Copy Markdown
Member

numba_smoketest_cuda_yaml_200 <-- Build Farm ID.

@gmarkall gmarkall left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Looking at the changes in https://github.com/numba/numba/pull/6878/files#diff-be83bb737c3b43603dfd2780d692709f3a95b36b9b86b77b88172404b97fab5c I am concerned I might have missed a case or two where spurious transfers could be induced in my haste to make this PR. I need to check whether the other modified cases in that PR could also induce unneeded transfers.

Comment on lines +242 to +243
setattr(driver, 'cuMemcpyHtoD', raising_transfer)
setattr(driver, 'cuMemcpyDtoH', raising_transfer)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggestion for future (when not in a hurry), try unittest.mock

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I do use it in some other places but I wasn't quite sure I could reliably replace the function on the driver singleton and remove it again afterwards if it wasn't present before with mock.

Comment on lines +108 to +111
dtype = getattr(ary, 'dtype')
if dtype is None:
dtype = np.asarray(ary).dtype
self.argtypes[i] = dtype

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see. The np.asarray is forcing a transfer to the host while we only care about getting the .dtype.

if old_HtoD is not None:
setattr(driver, 'cuMemcpyHtoD', old_HtoD)
else:
del driver.cuMemcpyHtoD_v2

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why are we deleting the _v2 version but we never have set it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's a mistake, thanks for catching it.

Comment on lines +245 to +251
try:
@vectorize(['float32(float32)'],>
def func(noise):
return noise + 1.0

func(noise)
finally:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll suggest adding a case where raising_transfer is called and the CudaAPIError is raised so we know the monkeypatch is working as expected.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point!

- Documentation via comments.
- Add checks that the mock is working correctly.
- Fix the names of members we delete (no `_v2`).
@gmarkall

gmarkall commented Jun 6, 2023

Copy link
Copy Markdown
Member Author

gpuci run tests

@gmarkall

gmarkall commented Jun 6, 2023

Copy link
Copy Markdown
Member Author

Having looked at this again I don't think I overlooked a case, I misunderstood the original changes in my earlier comment. So I think this is now ready for another look @sklam - many thanks for the first review!

@gmarkall gmarkall added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 3 - Ready for Review labels Jun 6, 2023
@esc

esc commented Jun 7, 2023

Copy link
Copy Markdown
Member

Build Farm ID: numba_smoketest_cuda_yaml_202

@esc

esc commented Jun 7, 2023

Copy link
Copy Markdown
Member

After merging #9004 numba_smoketest_cuda_yaml_202 has passed.

@sklam sklam added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Jun 7, 2023
@esc
esc merged commit c9cc06b into numba:main Jun 7, 2023
esc added a commit to esc/numba that referenced this pull request Jun 7, 2023
Fix: Issue numba#8923 - avoid spurious device-to-host transfers in CUDA ufuncs
This was referenced Jun 7, 2023
esc added a commit to esc/numba that referenced this pull request Jun 28, 2023
```
 💣 zsh» git --no-pager log -1 --decorate --oneline
04e8107 (HEAD -> release0.57, tag: 0.57.1, origin/release0.57) Merge pull request numba#9026 from esc/change_log_etc_0.57.1

 💣 zsh» git --no-pager log -1 --decorate --oneline main
82d3cbb (origin/main, origin/HEAD, main) Merge pull request numba#8911 from guilhermeleobas/guilhermeleobas/remoeisinstance-feat-warn

 💣 zsh» git --no-pager diff main -- CHANGE_LOG docs/source/user/installing.rst
diff --git c/CHANGE_LOG w/CHANGE_LOG
index 8e57301..7efe83d 100644
--- c/CHANGE_LOG
+++ w/CHANGE_LOG
@@ -1,3 +1,30 @@
+Version 0.57.1 (21 June, 2023)
+------------------------------
+
+Pull-Requests:
+
+* PR `numba#8964 <https://github.com/numba/numba/pull/8964>`_: fix missing nopython keyword in cuda random module (`esc <https://github.com/esc>`_)
+* PR `numba#8965 <https://github.com/numba/numba/pull/8965>`_: fix return dtype for np.angle (`guilhermeleobas <https://github.com/guilhermeleobas>`_ `esc <https://github.com/esc>`_)
+* PR `numba#8982 <https://github.com/numba/numba/pull/8982>`_: Don't do the parfor diagnostics pass for the parfor gufunc. (`DrTodd13 <https://github.com/DrTodd13>`_)
+* PR `numba#8996 <https://github.com/numba/numba/pull/8996>`_: adding a test for 8940 (`esc <https://github.com/esc>`_)
+* PR `numba#8958 <https://github.com/numba/numba/pull/8958>`_: resurrect the import, this time in the registry initialization (`esc <https://github.com/esc>`_)
+* PR `numba#8947 <https://github.com/numba/numba/pull/8947>`_: Introduce internal _isinstance_no_warn (`guilhermeleobas <https://github.com/guilhermeleobas>`_ `esc <https://github.com/esc>`_)
+* PR `numba#8998 <https://github.com/numba/numba/pull/8998>`_: Fix 8939 (second attempt) (`esc <https://github.com/esc>`_)
+* PR `numba#8978 <https://github.com/numba/numba/pull/8978>`_: Import MVC packages when using MVCLinker. (`bdice <https://github.com/bdice>`_)
+* PR `numba#8895 <https://github.com/numba/numba/pull/8895>`_: CUDA: Enable caching functions that use CG (`gmarkall <https://github.com/gmarkall>`_)
+* PR `numba#8976 <https://github.com/numba/numba/pull/8976>`_: Fix index URL for ptxcompiler/cubinlinker packages. (`bdice <https://github.com/bdice>`_)
+* PR `numba#9004 <https://github.com/numba/numba/pull/9004>`_: Skip MVC test when libraries unavailable (`gmarkall <https://github.com/gmarkall>`_ `esc <https://github.com/esc>`_)
+* PR `numba#9006 <https://github.com/numba/numba/pull/9006>`_: link to version support table instead of using explicit versions (`esc <https://github.com/esc>`_)
+* PR `numba#9005 <https://github.com/numba/numba/pull/9005>`_: Fix: Issue numba#8923 - avoid spurious device-to-host transfers in CUDA ufuncs (`gmarkall <https://github.com/gmarkall>`_)
+
+
+Authors:
+
+* `bdice <https://github.com/bdice>`_
+* `DrTodd13 <https://github.com/DrTodd13>`_
+* `esc <https://github.com/esc>`_
+* `gmarkall <https://github.com/gmarkall>`_
+
 Version 0.57.0 (1 May, 2023)
 ----------------------------

@@ -395,6 +422,7 @@ Pull-Requests:
 * PR `numba#8879 <https://github.com/numba/numba/pull/8879>`_: Remove use of ``compile_isolated`` from generator tests. (`stuartarchibald <https://github.com/stuartarchibald>`_)
 * PR `numba#8880 <https://github.com/numba/numba/pull/8880>`_: Fix missing dependency guard on pyyaml in ``test_azure_config``. (`stuartarchibald <https://github.com/stuartarchibald>`_)
 * PR `numba#8881 <https://github.com/numba/numba/pull/8881>`_: Replace use of compile_isolated in test_obj_lifetime (`sklam <https://github.com/sklam>`_)
+* PR `numba#8884 <https://github.com/numba/numba/pull/8884>`_: Pin llvmlite and NumPy on release branch (`sklam <https://github.com/sklam>`_)
 * PR `numba#8887 <https://github.com/numba/numba/pull/8887>`_: Update PyPI supported version tags (`bryant1410 <https://github.com/bryant1410>`_)
 * PR `numba#8896 <https://github.com/numba/numba/pull/8896>`_: Remove codecov install (now deleted from PyPI) (`gmarkall <https://github.com/gmarkall>`_)
 * PR `numba#8902 <https://github.com/numba/numba/pull/8902>`_: Enable CALL_FUNCTION_EX fix for py3.11 (`sklam <https://github.com/sklam>`_)
diff --git c/docs/source/user/installing.rst w/docs/source/user/installing.rst
index 72307ac..a83c4fd 100644
--- c/docs/source/user/installing.rst
+++ w/docs/source/user/installing.rst
@@ -262,6 +262,8 @@ information.
 +----------++--------------+---------------------------+----------------------------+------------------------------+-------------------+-----------------------------+
 | Numba     | Release date | Python                    | NumPy                      | llvmlite                     | LLVM              | TBB                         |
 +===========+==============+===========================+============================+==============================+===================+=============================+
+| 0.57.1    | 2023-06-21   | 3.8.x <= version < 3.12   | 1.21 <= version < 1.25     | 0.40.x                       | 14.x              | 2021.6 <= version           |
++-----------+--------------+---------------------------+----------------------------+------------------------------+-------------------+-----------------------------+
 | 0.57.0    | 2023-05-01   | 3.8.x <= version < 3.12   | 1.21 <= version < 1.25     | 0.40.x                       | 14.x              | 2021.6 <= version           |
 +-----------+--------------+---------------------------+----------------------------+------------------------------+-------------------+-----------------------------+
 | 0.56.4    | 2022-11-03   | 3.7.x <= version < 3.11   | 1.18 <= version < 1.24     | 0.39.x                       | 11.x              | 2021.x                      |
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 - Ready to merge Review and testing done, is ready to merge CUDA CUDA related issue/PR Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants