Skip to content
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

Fix Numba serialization when strides is None #3166

Merged
merged 4 commits into from Oct 24, 2019

Conversation

pentschev
Copy link
Member

This is necessary for newer Numba versions with __cuda_array_interface__ version 2.

The existing test fails for Numba 0.46.0, so this case is automatically covered, but I'm not sure if we can really have a specific test for this.

cc @quasiben @madsbk

@pentschev
Copy link
Member Author

The failure is unrelated to this PR.

itemsize = np.dtype(header["typestr"]).itemsize
strides = list(header["shape"][1:]) + [itemsize]
for i in range(len(strides) - 1, 0, -1):
strides[i - 1] *= strides[i]
Copy link
Member

Choose a reason for hiding this comment

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

You might want something like the following?

strides = np.cumprod(shape[::-1]) * itemsize

Copy link
Member

Choose a reason for hiding this comment

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

Why didn't this show up in tests? Don't we test with a C-contiguous array?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, it's indeed what I wanted and couldn't think of when I wrote the code.

There are two reasons it didn't show up in the tests:

  1. CUDA code is not being tested by CI;
  2. Numba >= 0.46.0 is required (which contains version 2 of __cuda_array_interface__.

@TomAugspurger
Copy link
Member

Did strides = np.cumprod(shape[::-1]) * itemsize fail for some cases?

Looks like the linting failure is real: https://travis-ci.org/dask/distributed/jobs/601702095#L491.

@pentschev
Copy link
Member Author

Thanks @TomAugspurger, I hadn't seen the latest failure. The new commit should fix it, let's hope it's green now!

@pentschev
Copy link
Member Author

Lint checks passed now, but CI failed again on unrelated tests.

@mrocklin mrocklin merged commit 876bca0 into dask:master Oct 24, 2019
@mrocklin
Copy link
Member

Thanks @pentschev ! Merging in.

@pentschev pentschev deleted the fix-numba-strides branch November 11, 2020 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants