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.

Sign up for GitHub

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

Jump to bottom

Allow MatDescriptor to be pickle-able #3157

Merged
merged 8 commits into from
Mar 11, 2020
Merged

Allow MatDescriptor to be pickle-able #3157

merged 8 commits into from
Mar 11, 2020

Conversation

jakirkham
Copy link
Member

@jakirkham jakirkham commented Mar 5, 2020 •
edited
Loading

Fixes #3061

@jakirkham jakirkham force-pushed the pickle_MatDescriptor branch from 31062d0 to 2d2209b Compare March 5, 2020 20:36
@jakirkham jakirkham mentioned this pull request Mar 5, 2020
Closed
@jakirkham
Copy link
Member Author

(note to self: still needs tests)

@jakirkham
Copy link
Member Author

@emcastillo, could you or someone else please help us figure out where to put a test for this? Am not seeing any tests for MatDescriptor currently

@jakirkham
Copy link
Member Author

FWIW I pushed some tests for pickling sparse matrices, which should exercise this a bit.

@jakirkham
Test for a new MatDescriptor after pickling
97c7bc1 
As `MatDescriptor` contains a pointer to a `cusparseMatDescr_t` object,
it will not be valid after pickling and reloaded in another process. So
test that a new pointer is given on unpickling. This will indicate that
`cusparseCreateMatDescr` was run again as part of unpickling.

ref: https://docs.nvidia.com/cuda/cusparse/index.html#cusparseCreateMatDescr
@jakirkham
Copy link
Member Author

Also may need some pointers on how testing is working here. I think I got the idea, but there were some differences between test modules. So could use some pointers from someone more familiar 🙂

@leofang
Copy link
Member

leofang commented Mar 6, 2020

John, if your new tests pass they should be fine. (I didn't review very carefully!) They are moving away from using assertEqual() to plain assert statements, see https://docs-cupy.chainer.org/en/stable/contribution.html#how-to-write-tests, so for now the test suite has a mixed old- and new-style tests.

@jakirkham
Copy link
Member Author

Thanks Leo! 😄

That's good to hear. assert feels more friendly 😉

Yeah was noticing that some of these use a _make function to get a sparse array. So tried to use that. Not sure if this is preferred or not.

The other thing is some tests seem to have arguments for xp and sp. Tried to support these, but not sure if it was supported in all modules or if there was anything extra I should be doing to make sure they work.

@emcastillo
Copy link
Member

I think that current tests are fine

@emcastillo emcastillo self-assigned this Mar 6, 2020
emcastillo
emcastillo approved these changes Mar 9, 2020
View reviewed changes
Copy link
Member

@emcastillo emcastillo left a comment

Choose a reason for hiding this comment

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

LGTM

@emcastillo emcastillo added the st:test-and-merge (deprecated) Ready to merge after test pass. label Mar 9, 2020
@emcastillo emcastillo added this to the v8.0.0b1 milestone Mar 9, 2020
@emcastillo
Copy link
Member

Jenkins, test this please

@pfn-ci-bot
Copy link
Collaborator

Successfully created a job for commit 7c96b16:

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 7c96b16, target branch master) failed with status FAILURE.

@emcastillo emcastillo added cat:bug Bugs and removed st:test-and-merge (deprecated) Ready to merge after test pass. labels Mar 10, 2020
@emcastillo
Copy link
Member

Jenkins, test this please

@pfn-ci-bot
Copy link
Collaborator

Successfully created a job for commit 39264af:

@emcastillo emcastillo added the st:test-and-merge (deprecated) Ready to merge after test pass. label Mar 10, 2020
@jakirkham
Only keep descriptor test in csc and csr
d0b5090 
Only `_compressed_sparse_matrix` subclasses include `MatDescriptor`.
Other sparse matrices don't. So drop this test from them.
@jakirkham
Use SciPy in sparse matrix pickle test comparisons
77bd1eb 
As it appears the comparisons don't work with CuPy currently, perform
them with SciPy instead.
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 39264af, target branch master) failed with status FAILURE.

@emcastillo
Copy link
Member

Jenkins, test this please

@pfn-ci-bot
Copy link
Collaborator

Successfully created a job for commit 77bd1eb:

@jakirkham
Copy link
Member Author

Sorry missed one thing (now fixed). 😅 Think that is the last thing. 🍀

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 77bd1eb, target branch master) failed with status FAILURE.

@jakirkham
Copy link
Member Author

Just as an FYI the Jenkins CI status is for the commit before the latest one. So this needs a retest 🙂

@emcastillo
Copy link
Member

Jenkins, test this please

@pfn-ci-bot
Copy link
Collaborator

Successfully created a job for commit c2d40b3:

@chainer-ci
Copy link
Member

Jenkins CI test (for commit c2d40b3, target branch master) succeeded!

@mergify mergify bot merged commit 081cb6a into cupy:master Mar 11, 2020
@jakirkham jakirkham deleted the pickle_MatDescriptor branch March 11, 2020 03:26
@jakirkham
Copy link
Member Author

Thanks for the reviews! 😄

@jakirkham
Copy link
Member Author

@kmaehashi, would it be ok to backport this?

jakirkham added a commit to jakirkham/cupy that referenced this pull request Aug 12, 2020
@jakirkham
[Backport] Allow MatDescriptor to be pickle-able
ff134e8 
Squashed commit of the following:

commit 081cb6a
Merge: c1cec3d c2d40b3
Author: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Date:   Wed Mar 11 03:02:49 2020 +0000

    Merge pull request cupy#3157 from jakirkham/pickle_MatDescriptor

    Allow `MatDescriptor` to be pickle-able

commit c2d40b3
Author: John Kirkham <[email protected]>
Date:   Tue Mar 10 01:48:18 2020 -0700

    Assert no mismatches found in sparse pickle test

commit 77bd1eb
Author: John Kirkham <[email protected]>
Date:   Tue Mar 10 01:40:43 2020 -0700

    Use SciPy in sparse matrix pickle test comparisons

    As it appears the comparisons don't work with CuPy currently, perform
    them with SciPy instead.

commit d0b5090
Author: John Kirkham <[email protected]>
Date:   Tue Mar 10 01:36:21 2020 -0700

    Only keep descriptor test in csc and csr

    Only `_compressed_sparse_matrix` subclasses include `MatDescriptor`.
    Other sparse matrices don't. So drop this test from them.

commit 39264af
Author: John Kirkham <[email protected]>
Date:   Tue Mar 10 00:52:33 2020 -0700

    Just test CuPy sparse matrices in pickle tests

commit 7c96b16
Author: John Kirkham <[email protected]>
Date:   Sun Mar 8 15:49:49 2020 -0700

    Add a few tests for `MatDescriptor` directly

commit 97c7bc1
Author: John Kirkham <[email protected]>
Date:   Thu Mar 5 19:03:32 2020 -0800

    Test for a new `MatDescriptor` after pickling

    As `MatDescriptor` contains a pointer to a `cusparseMatDescr_t` object,
    it will not be valid after pickling and reloaded in another process. So
    test that a new pointer is given on unpickling. This will indicate that
    `cusparseCreateMatDescr` was run again as part of unpickling.

    ref: https://docs.nvidia.com/cuda/cusparse/index.html#cusparseCreateMatDescr

commit 3524103
Author: John Kirkham <[email protected]>
Date:   Thu Mar 5 18:58:22 2020 -0800

    Test pickling sparse matrices

commit 2d2209b
Author: John Kirkham <[email protected]>
Date:   Thu Mar 5 12:36:14 2020 -0800

    Allow `MatDescriptor` to be pickle-able
@jakirkham jakirkham mentioned this pull request Aug 12, 2020
Merged
@jakirkham
Copy link
Member Author

Backporting with PR ( #3771 ).

@kmaehashi kmaehashi added the to-be-backported Pull-requests to be backported to stable branch label Aug 12, 2020
@jakirkham jakirkham mentioned this pull request May 15, 2023
Merged
2 tasks
@charlesbluca charlesbluca mentioned this pull request May 30, 2023
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leofang leofang leofang left review comments

@emcastillo emcastillo emcastillo approved these changes

Assignees

@emcastillo emcastillo

Labels
cat:bug Bugs st:test-and-merge (deprecated) Ready to merge after test pass. to-be-backported Pull-requests to be backported to stable branch
Projects
None yet
Milestone
v8.0.0b1
Development

Successfully merging this pull request may close these issues.

cupy.sparse.MatDescriptor is not pickleable
6 participants
@jakirkham @leofang @emcastillo @pfn-ci-bot @chainer-ci @kmaehashi