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 trying to get pointer to None in svm/linear.pyx #5615

Merged
merged 4 commits into from
Oct 31, 2023

Conversation

yosider
Copy link
Contributor

@yosider yosider commented Oct 10, 2023

This PR fixes the following bug in svm/linear.pyx:

After fitting LinearSVC and saving it using pickle, attempting to load and run predict() (or decision_function() and predict_proba() that call __sync_model()) results in the following error:

AttributeError                            Traceback (most recent call last)
----> 1 lin_svc_load.predict(X)

File .../lib/python3.10/site-packages/cuml/internals/api_decorators.py:188, in _make_decorator_function.<locals>.decorator_function.<locals>.decorator_closure.<locals>.wrapper(*args, **kwargs)
    185     set_api_output_dtype(output_dtype)
    187 if process_return:
--> 188     ret = func(*args, **kwargs)
    189 else:
    190     return func(*args, **kwargs)

File linear.pyx:693, in cuml.svm.linear.LinearSVM.predict()

File linear.pyx:667, in cuml.svm.linear.LinearSVM.__sync_model()

File linear.pyx:303, in cuml.svm.linear.LinearSVMWrapper.__cinit__()

AttributeError: 'NoneType' object has no attribute 'ptr'

I identified that the issue is caused by attempting to obtain a pointer to a variable with a None value in LinearSVMWrapper.__cinit__(). After fixing this and rebuilding, the code worked as expected.

Minimal code to reproduce the bug:

import pickle
import numpy as np
from cuml.svm import LinearSVC

X = np.random.randn(10, 2)
y = np.random.randint(0, 2, 10)

svc = LinearSVC()
svc.fit(X, y)

with open("tmp.pkl", "wb") as f:
    pickle.dump(svc, f)
with open("tmp.pkl", "rb") as f:
    svc_load = pickle.load(f)

svc_load.predict(X)

If this is not the appropriate way to fix the issue, I would appreciate any corrections.

@yosider yosider requested a review from a team as a code owner October 10, 2023 11:25
@copy-pr-bot
Copy link

copy-pr-bot bot commented Oct 10, 2023

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions bot added the Cython / Python Cython or Python issue label Oct 10, 2023
@csadorf csadorf added bug Something isn't working non-breaking Non-breaking change labels Oct 11, 2023
@csadorf
Copy link
Contributor

csadorf commented Oct 11, 2023

/ok to test

Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

@yosider Thanks a lot for reporting the issue, providing an MRE and even a proposed fix!

Before we move forward, I think it would be very important that we add a test that captures this bug. I would proposed to add a unit test to the python/cuml/tests/test_pickle.py module for the LinearSVC class similar to some of the other tests. Please let me know if you need more guidance!

@yosider
Copy link
Contributor Author

yosider commented Oct 12, 2023

@csadorf Thanks for your advice!
I've added test_linear_svc_pickle() based on test_svc_pickle().
It seems that LinearSVC does not support sparse data, so I've excluded tests for sparse cases.

@csadorf
Copy link
Contributor

csadorf commented Oct 18, 2023

/ok to test

Copy link
Contributor

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks a lot!

@csadorf
Copy link
Contributor

csadorf commented Oct 18, 2023

@yosider I'll look into the CI test failures which are almost certainly unrelated.

@dantegd
Copy link
Member

dantegd commented Oct 31, 2023

/ok to test

@dantegd
Copy link
Member

dantegd commented Oct 31, 2023

/merge

@rapids-bot rapids-bot bot merged commit a01181f into rapidsai:branch-23.12 Oct 31, 2023
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Cython / Python Cython or Python issue non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants