Skip to content

misc: jit: Deprecate load_cuda_ops() #1066

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
Merged
merged 1 commit into from
May 21, 2025
Merged

misc: jit: Deprecate load_cuda_ops() #1066

merged 1 commit into from
May 21, 2025

Conversation

abcdabcd987
Copy link
Member

@abcdabcd987 abcdabcd987 commented May 19, 2025 •
edited
Loading

Part of AOT Refactor (#1064).

This PR changes the caller of load_cuda_ops() to directly use JitSpec. Also add deprecation warning for load_cuda_ops().

@abcdabcd987 abcdabcd987 changed the base branch from main to lequn/jit-spec May 19, 2025 07:19
@abcdabcd987 abcdabcd987 mentioned this pull request May 19, 2025
Closed
10 tasks
@abcdabcd987 abcdabcd987 requested a review from yzh119 May 19, 2025 07:22
yzh119
yzh119 reviewed May 20, 2025
View reviewed changes
Copy link
Collaborator

@yzh119 yzh119 left a comment

Choose a reason for hiding this comment

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

Would you mind checking the failed case in
https://ci.tlcpack.ai/blue/organizations/jenkins/flashinfer-ci/detail/lequn%2Fremove-load_cuda_ops/2/pipeline/16

[2025-05-19T07:21:06.255Z] !!!!! _pytest.outcomes.Exit: 'tuple' object has no attribute 'ninja_path' !!!!!!

@abcdabcd987 abcdabcd987 force-pushed the lequn/remove-load_cuda_ops branch from 7cf6b0f to 3274066 Compare May 20, 2025 18:58
yzh119
yzh119 reviewed May 21, 2025
View reviewed changes
Copy link
Collaborator

@yzh119 yzh119 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can we still keep load_cuda_ops for a while? As some of the other PRs still depends on it.

@abcdabcd987
Copy link
Member Author

Can you point to other use cases for load_cuda_ops? It looks to me that it'd be fairly easy to switch to the new pattern -- gen_jit_spec(...).build_and_load(). And ideally, gen JitSpec and build_and_load() should happen in two different places, for example, gen_xxx_module() and get_xxx_module(). By removing load_cuda_ops, I'd like to make this pattern change more obvious to future PRs.

@yzh119
Copy link
Collaborator

yzh119 commented May 21, 2025

Can you point to other use cases for load_cuda_ops

Coming from some of the PRs that have not been merged yet.

Base automatically changed from lequn/jit-spec to main May 21, 2025 20:26
@abcdabcd987 abcdabcd987 force-pushed the lequn/remove-load_cuda_ops branch from 3274066 to 939a30d Compare May 21, 2025 20:30
@abcdabcd987 abcdabcd987 changed the title misc: jit: remove load_cuda_ops() misc: jit: Deprecate load_cuda_ops() May 21, 2025
@abcdabcd987
Copy link
Member Author

Updated. Added deprecation warning.

yzh119
yzh119 approved these changes May 21, 2025
View reviewed changes
Copy link
Collaborator

@yzh119 yzh119 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@yzh119 yzh119 merged commit 8323a29 into main May 21, 2025
2 checks passed
@abcdabcd987 abcdabcd987 deleted the lequn/remove-load_cuda_ops branch May 21, 2025 21:37
Edenzzzz pushed a commit to Edenzzzz/flashinfer that referenced this pull request Jun 6, 2025
@abcdabcd987 @Edenzzzz
misc: jit: Deprecate load_cuda_ops() (flashinfer-ai#1066)
a61f8b3 
Part of AOT Refactor (flashinfer-ai#1064).

This PR changes the caller of `load_cuda_ops()` to directly use
`JitSpec`. Also add deprecation warning for `load_cuda_ops()`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yzh119 yzh119 yzh119 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.
2 participants
@abcdabcd987 @yzh119