Skip to content

Fix slicing bug in ensure_memoryview#6449

Merged
jrbourbeau merged 2 commits intodask:mainfrom
jakirkham:fix_6448
May 26, 2022
Merged

Fix slicing bug in ensure_memoryview#6449
jrbourbeau merged 2 commits intodask:mainfrom
jakirkham:fix_6448

Conversation

@jakirkham
Copy link
Copy Markdown
Member

@jakirkham jakirkham commented May 25, 2022 •
edited
Loading

Fixes #6448

The underlying issues seems to be related to slicing. This example demonstrates the issue below and has been included in the test suite.

from distributed.utils import ensure_memoryview

mv = memoryview(b"0123456789").cast("B", (5, 2))[1:-1]
mv2 = ensure_memoryview(mv)

assert mv == mv2
  • Tests added / passed
  • Passes pre-commit run --all-files

@jakirkham jakirkham mentioned this pull request May 25, 2022
Closed
@toloudis
Copy link
Copy Markdown

toloudis commented May 25, 2022

should ultimately update the comment above this code too

@jakirkham
Copy link
Copy Markdown
Member Author

jakirkham commented May 25, 2022

@JacksonMaxfield, would you be able to try this out? 🙂

@evamaxfield
Copy link
Copy Markdown

evamaxfield commented May 25, 2022

In class, gimme an hour or so and will do!

@jakirkham
Copy link
Copy Markdown
Member Author

jakirkham commented May 25, 2022

No rush. Thanks! 🙏

This was referenced May 25, 2022
Closed
Merged
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented May 25, 2022 •
edited
Loading

Unit Test Results

       15 files  ±  0         15 suites  ±0   6h 19m 34s ⏱️ - 1m 15s
  2 816 tests +  4    2 735 ✔️ +  3    79 💤 ±0  2 ❌ +1 
20 875 runs  +28  19 944 ✔️ +29  928 💤  - 3  3 ❌ +2 

For more details on these failures, see this check.

Results for commit e1ccade. ± Comparison against base commit 046ab17.

♻️ This comment has been updated with latest results.

@evamaxfield
Copy link
Copy Markdown

evamaxfield commented May 25, 2022 •
edited
Loading

@jakirkham here are the checks for this branch install: https://github.com/AllenCellModeling/aicsimageio/runs/6600770595?check_suite_focus=true

The important tests are the test-upstreams and so far it looks like they are all passing 🟢

Cannot believe this is less than a single line change to make it work. Fun.

@evamaxfield
Copy link
Copy Markdown

evamaxfield commented May 25, 2022

That said, maybe wait for macOS builds to finish up but your branch is working so far.

@jakirkham
Copy link
Copy Markdown
Member Author

jakirkham commented May 25, 2022

Thanks for checking Jackson! 🙏 Glad to hear it works 😄

@jakirkham
Copy link
Copy Markdown
Member Author

jakirkham commented May 25, 2022 •
edited
Loading

Trying to reproduce the issue myself so I can come up with a test. Tried to do

curl -LO https://open.quiltdata.com/b/aics-modeling-packages-test-resources/tree/aicsimageio/test_resources/resources/3d-cell-viewer.ome.tiff

But ran into some issues. Is there something else I need to do to download this file?

Edit: NVM think I figured this out

@evamaxfield
Copy link
Copy Markdown

evamaxfield commented May 25, 2022 •
edited
Loading

Hah, yea sorry that is just the viewer and UI downloader.

@jakirkham
Copy link
Copy Markdown
Member Author

jakirkham commented May 25, 2022

Yeah sorry poked around a bit and found the right URL. Now have it reproducing for me locally. Going to dig into it a bit more

@jakirkham
Drop .obj in PickleBuffer
3a4ceeb 
This causes problems with certain kinds of views. So just pass the
`memoryview` as-is even though `PickleBuffer` will chain the underlying
`memoryview`s instead of referencing the original `obj`. Maybe this can
be fixed upstream.
@jakirkham
Copy link
Copy Markdown
Member Author

jakirkham commented May 26, 2022

Ok found a test that covers a similar problem. Think it is likely the cause of the issue in the original repro, but it is a little difficult for me to parse fully atm. In any event, this test will fail without the change here, which should ensure we don't regress

@jakirkham jakirkham marked this pull request as ready for review May 26, 2022 08:12
@jakirkham jakirkham changed the title WIP: ensure_memoryview fix ensure_memoryview fix May 26, 2022
@jakirkham jakirkham changed the title ensure_memoryview fix Fix slicing bug in ensure_memoryview May 26, 2022
@jakirkham jakirkham requested a review from jrbourbeau May 26, 2022 08:22
@jakirkham jakirkham force-pushed the fix_6448 branch 2 times, most recently from db6653f to d7c56f9 Compare May 26, 2022 08:27
@jakirkham
Include tests for slicing issue
e1ccade 
This reproduces a similar issue to what is seen in the original repro.
Most importantly it fails if `mv.obj` is used.
@pentschev
Copy link
Copy Markdown
Member

pentschev commented May 26, 2022

rerun tests

Note: rerunning gpuCI tests since those errors should be fixed by #6434

@jakirkham
Copy link
Copy Markdown
Member Author

jakirkham commented May 26, 2022 •
edited
Loading

Restarted the failed GitHub Actions for good measure.

Edit: Though they looked like flaky tests.

jrbourbeau
jrbourbeau approved these changes May 26, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix @jakirkham and reporting the original issue @JacksonMaxfield. This LGTM, will merge once CI finishes

@jakirkham
Copy link
Copy Markdown
Member Author

jakirkham commented May 26, 2022

All of the tests passed except for the macOS test, which is still queued. Do we want to wait for that?

@jrbourbeau jrbourbeau merged commit 1db476c into dask:main May 26, 2022
@jrbourbeau
Copy link
Copy Markdown
Member

jrbourbeau commented May 26, 2022

The previous failure for that build was a flaky test, so happy to merge in

@jakirkham
Copy link
Copy Markdown
Member Author

jakirkham commented May 26, 2022

Thanks James! Also thanks Jackson for testing 🙏

@jakirkham jakirkham deleted the fix_6448 branch May 26, 2022 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jrbourbeau jrbourbeau jrbourbeau approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug introduced in distributed 2022.5.1 -- unpack_frames

5 participants

@jakirkham @toloudis @evamaxfield @pentschev @jrbourbeau