-
-
Notifications
You must be signed in to change notification settings - Fork 748
Fix slicing bug in ensure_memoryview
#6449
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
Conversation
|
should ultimately update the comment above this code too |
|
@JacksonMaxfield, would you be able to try this out? 🙂 |
|
In class, gimme an hour or so and will do! |
|
No rush. Thanks! 🙏 |
Unit Test Results 15 files ± 0 15 suites ±0 6h 19m 34s ⏱️ - 1m 15s 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. |
|
@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 Cannot believe this is less than a single line change to make it work. Fun. |
|
That said, maybe wait for macOS builds to finish up but your branch is working so far. |
|
Thanks for checking Jackson! 🙏 Glad to hear it works 😄 |
|
Trying to reproduce the issue myself so I can come up with a test. Tried to do But ran into some issues. Is there something else I need to do to download this file? Edit: NVM think I figured this out |
|
Hah, yea sorry that is just the viewer and UI downloader. |
|
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 |
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.
|
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 |
db6653f to
d7c56f9
Compare
This reproduces a similar issue to what is seen in the original repro. Most importantly it fails if `mv.obj` is used.
|
rerun tests Note: rerunning gpuCI tests since those errors should be fixed by #6434 |
|
Restarted the failed GitHub Actions for good measure. Edit: Though they looked like flaky tests. |
jrbourbeau
left a comment
There was a problem hiding this 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
|
All of the tests passed except for the macOS test, which is still queued. Do we want to wait for that? |
|
The previous failure for that build was a flaky test, so happy to merge in |
|
Thanks James! Also thanks Jackson for testing 🙏 |
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.
pre-commit run --all-files