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

Correct arguments to load_from_treelite_model after classmethod conversion #5210

Merged
merged 2 commits into from
Feb 7, 2023
Merged

Correct arguments to load_from_treelite_model after classmethod conversion #5210

merged 2 commits into from
Feb 7, 2023

Conversation

wphicks
Copy link
Contributor

@wphicks wphicks commented Feb 6, 2023

No description provided.

@wphicks wphicks requested a review from a team as a code owner February 6, 2023 22:41
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Feb 6, 2023
viclafargue
viclafargue approved these changes Feb 7, 2023
View reviewed changes
Copy link
Contributor

@viclafargue viclafargue left a comment

Choose a reason for hiding this comment

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

LGTM

csadorf
csadorf reviewed Feb 7, 2023
View reviewed changes
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.

The function you are patching is still declared to be a staticmethod which seems to contradict what you are saying in the PR title. What am I missing?

@wphicks wphicks changed the base branch from branch-23.04 to branch-23.02 February 7, 2023 16:43
@wphicks
Copy link
Contributor Author

wphicks commented Feb 7, 2023

@csadorf Sorry, I'm not seeing the string staticmethod anywhere in fil.pyx at this point. Can you point me to what function you mean?

@wphicks wphicks added bug Something isn't working ! - Hotfix Hotfix is a bug that affects the majority of users for which there is no reasonable workaround non-breaking Non-breaking change labels Feb 7, 2023
@raydouglass raydouglass merged commit d444099 into rapidsai:branch-23.02 Feb 7, 2023
@csadorf
Copy link
Contributor

csadorf commented Feb 8, 2023

@csadorf Sorry, I'm not seeing the string staticmethod anywhere in fil.pyx at this point. Can you point me to what function you mean?

@wphicks https://github.com/rapidsai/cuml/pull/5210/files/028b8c3df4f57ab261fb86e4c530d9c0e4c1a856#diff-db50ff6dd40b3f8a631e079e0a02a07c335c5e28c2a7c8fe4509e46782883b17R846 line 847

I assume the discrepancy resulted from this PR previously targeting the wrong branch.

jakirkham pushed a commit to jakirkham/cuml that referenced this pull request Feb 27, 2023
@wphicks
Do not blindly push all locals into kwargs (rapidsai#5210)
821bece 
Authors:
   - William Hicks (https://github.com/wphicks)

Approvers:
   - Victor Lafargue (https://github.com/viclafargue)
   - Dante Gama Dessavre (https://github.com/dantegd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csadorf csadorf csadorf left review comments

@viclafargue viclafargue viclafargue approved these changes

@dantegd dantegd dantegd approved these changes

Assignees
No one assigned
Labels
! - Hotfix Hotfix is a bug that affects the majority of users for which there is no reasonable workaround bug Something isn't working Cython / Python Cython or Python issue non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wphicks @csadorf @dantegd @viclafargue @raydouglass