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

[TIR][Schedule] Remove @type_check for set_axis_separator #17134

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

abhikran-quic
Copy link
Contributor

@abhikran-quic abhikran-quic commented Jul 3, 2024

The decorator is not allowing types like Array to be passed to set_axis_separator directive.
The FFI has a type checking implemented internally making this redundant.
@abhikran-quic
Copy link
Contributor Author

@tvm-bot rerun

@abhikran-quic
Copy link
Contributor Author

cc: @Lunderberg
I've tried to address a review comment in #17115 where we discussed about @type_check in set_axis_separator.
Please help in reviewing this change. Thank you!

@abhikran-quic abhikran-quic changed the title [TIR][Schedule] Remove @type_check decorator for set_axis_separator [TIR][Schedule] Remove @type_check for set_axis_separator Jul 6, 2024
Copy link
Contributor

@Lunderberg Lunderberg left a comment

Choose a reason for hiding this comment

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

Thank you for the follow-up, and this change looks good! I like the consolidation of the type-checking into the FFI.

@abhikran-quic
Copy link
Contributor Author

Thank you for the follow-up, and this change looks good! I like the consolidation of the type-checking into the FFI.

Thank you @Lunderberg for reviewing this change!
I saw other scheduling directives using type_check . Do you recommend cleaning up the others as well ? If yes, I can volunteer to do it.

@Lunderberg
Copy link
Contributor

I saw other scheduling directives using type_check . Do you recommend cleaning up the others as well ? If yes, I can volunteer to do it.

If they are being checked in the FFI, then that would be fantastic, thank you! The main thing to watch out for is C++ functions that use set_body_typed and accept ObjectRef, or which use set_body to interact with the PackedFunc interface directly. Neither of these forms apply a strict type-checking during the FFI, so the python-side checks might still be useful there.

@abhikran-quic
Copy link
Contributor Author

I saw other scheduling directives using type_check . Do you recommend cleaning up the others as well ? If yes, I can volunteer to do it.

If they are being checked in the FFI, then that would be fantastic, thank you! The main thing to watch out for is C++ functions that use set_body_typed and accept ObjectRef, or which use set_body to interact with the PackedFunc interface directly. Neither of these forms apply a strict type-checking during the FFI, so the python-side checks might still be useful there.

Sure. I will work on this and try to check how much more type checks can be moved to FFI in a new PR. Would it be okay to merge this PR ?
Thank you!

@Lunderberg Lunderberg merged commit fd7c81d into apache:main Jul 9, 2024
21 checks passed
@abhikran-quic abhikran-quic deleted the type_check branch July 10, 2024 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants