core: deprecate Block.from_callable and replace with builder/implicit builder #1119
Conversation
superlopuh
requested review from
webmiche,
compor and
georgebisbas
as code owners
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1119 +/- ##
==========================================
+ Coverage 87.01% 87.03% +0.02%
==========================================
Files 137 137
Lines 20793 20845 +52
Branches 3131 3132 +1
==========================================
+ Hits 18093 18143 +50
- Misses 2181 2184 +3
+ Partials 519 518 -1
☔ View full report in Codecov by Sentry. |
| @@ -1118,6 +1118,7 @@ class BlockCallback(Protocol): | |||
| def __call__(self, *args: BlockArgument) -> list[Operation]: | |||
| ... | |||
|
|
|||
| @deprecated("Please use Builder instead") | |||
There was a problem hiding this comment.
I'm not sure we want to deprecate this now?
At least, I don't see any replacement for it, as the only way is to either do it by hand or create an implicit builder.
Can we instead make BlockCallback take a builder as argument or something?
There was a problem hiding this comment.
But there is a replacement for it, which is to create it by hand or use an implicit builder :)
There was a problem hiding this comment.
I'd like to delete BlockCallback as soon as clients have migrated off
There was a problem hiding this comment.
Well, I think that's a problem to have only these two options. I feel that in general, we should have
- The hand-way, which realistically no one should use
- The manual builder way, which is the MLIR-like way
- The implicit builder way, which is the nice xDSL way
But I'm actually fine with this PR, since none of these cases should require the from_callable
There was a problem hiding this comment.
Hmm I am not 100% convinced yet that we want to do this. To me, the BlockCallback things are mainly useful as a sort of nicer syntax for creating blocks with arguments. I see that the implicit builder is an even better syntax, so I think the core Idea is a good one.
What I am uncertain (probably because I am a bit out of the loop on the builder) is whether we can have the same kinds of constructs in a builder as in a general Python functions. In particular, can we handle general python control flow (like say a loop adding dependent operations or an if-clause) in a nice, straightforward way?
|
Yeah, they are all possible, as far as I can tell they're exactly equivalent. Ultimately they're both sugar around adding operations to a list of operations, in my understanding, hard to imagine where one would be more expressive than the other. |
… builder (xdslproject#1119) This would unify the API a bit, and redirect users towards the Builders. I think the Builder API is ready for adoption, and the making the building more uniform would make the project more approachable.
This would unify the API a bit, and redirect users towards the Builders. I think the Builder API is ready for adoption, and the making the building more uniform would make the project more approachable.