Skip to content

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
@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