search

tlc-pack / relax

Apache License 2.0
193 stars 58 forks source link

[Op] Remove MatchCast requirement for manipulation ops #436

Closed MasterJH5574 closed 1 year ago

MasterJH5574 commented 1 year ago

Prior to this PR, the structure info inference of some manipulation operators rejects unknown ndim or unknown shape for safety reason. As many people are pointing out, this requirement increases the overhead of our frontend importers, in the way of forcing the importers to use MatchCast, which turns out to be ineffective and troublesome.

Therefore, this PR removes such requirements, turning into the behavior of optimistically trust the input that they have the desired ndim or shape when those properties are unknown.

Nevertheless, this PR leaves TODO items at such places, which serve as reminders for us to support corresponding runtime ndim or shape check in the future.

slyubomirsky commented 1 year ago

I think it's fine to have a policy of having operators dynamically check shapes instead of demanding a MatchCast first. However, the resulting StructInfo should be consistent and indicate a statically unknown shape (so a MatchCast would be needed on the result if the shape is needed elsewhere).

Edit: To be clear, it would be important to have the ops still check the input shapes and fail cleanly if the shape at run time is inappropriate. Otherwise, there would be a lot of potential for chaos.

Edit 2: We could even enforce this with some "negative" test cases (checking that the op will fail if dynamically passed an input with the wrong shape).

MasterJH5574 commented 1 year ago

I think it's fine to have a policy of having operators dynamically check shapes instead of demanding a MatchCast first. However, the resulting StructInfo should be consistent and indicate a statically unknown shape (so a MatchCast would be needed on the result if the shape is needed elsewhere).

Edit: To be clear, it would be important to have the ops still check the input shapes and fail cleanly if the shape at run time is inappropriate. Otherwise, there would be a lot of potential for chaos.

Edit 2: We could even enforce this with some "negative" test cases (checking that the op will fail if dynamically passed an input with the wrong shape).

@slyubomirsky Yes we have plans to do runtime shape/ndim checks for these cases in the future (and the TODOs prior to this PR already mean that). This PR is mainly to remove the constraint so that we don’t enforce the frontend importer to insert MatchCasts.

tqchen commented 1 year ago

cc @jwfromm