core: Move MLContext out of ir, move get_all_dialects to dialects. #2749
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
PapyChacal
force-pushed
the
emilien/context
branch
from
83f388a to
8499571
Compare
PapyChacal
force-pushed
the
emilien/context
branch
from
8499571 to
bf04b6d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2749 +/- ##
=======================================
Coverage 89.77% 89.77%
=======================================
Files 369 371 +2
Lines 47545 47608 +63
Branches 7291 7292 +1
=======================================
+ Hits 42684 42742 +58
- Misses 3725 3728 +3
- Partials 1136 1138 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Really nice!
I would just have a question relative to MLContext.register_all_dialects.
To me, this is a bit weird, as we do not really want users to overload MLContext I think? Or at least this is not how I was currently seeing it, and should change my view.
But to me, having the register_all_dialects in the command line tool made sense as it was a tool that was meant to be inherited for each use case. However, for the MLContext, I don't see why we would overload this for each tool besides to add these dialects (which we can add in another way?).
But maybe I'm just missing a use case you have on this?
Otherwise, thanks a lot for moving the get_all_dialects!!!!
| @@ -0,0 +1,310 @@ | |||
| from collections.abc import Callable | |||
There was a problem hiding this comment.
Oh, I would not have thought about adding this to the __init__.py directly, but that's a really good idea!
xdsl/context.py
Outdated
| def register_all_dialects(self): | ||
| """ | ||
| Register all dialects that can be used. | ||
|
|
||
| Add other/additional dialects by overloading this function. | ||
| """ | ||
| for dialect_name, dialect_factory in get_all_dialects().items(): | ||
| self.register_dialect(dialect_name, dialect_factory) |
There was a problem hiding this comment.
I am not entirely sure that anyone should inherit MLContext, so I would drop this comment.
Which is also why I'm not sure we should add this method here? Maybe we should just have register_dialects to make this simpler and just have context.register_dialects(get_all_dialects())? Or keep what we had before.
At least, I would prefer keep this a conscious decision for the user to add all dialects or not, and for that asking them to call get_all_dialects make sense to me if that makes sense? I don't know, what do you think?
There was a problem hiding this comment.
I think this gets especially weird for users of xDSL that want to add their own dialects, it seems a little cleaner to let them do their own combinations of different dialects rather than strongly encouraging inheritance in this way.
xdsl/tools/command_line_tool.py
Outdated
| def register_all_dialects(self): | ||
| """ | ||
| Register all dialects that can be used. | ||
|
|
||
| Add other/additional dialects by overloading this function. | ||
| """ | ||
| for dialect_name, dialect_factory in get_all_dialects().items(): | ||
| self.ctx.register_dialect(dialect_name, dialect_factory) | ||
|
|
There was a problem hiding this comment.
I would still keep this here actually, and remove it from MLContext, what do you think?
This class is supposed to be overloaded for each tool, so adding the dialects here would make sense to me. While MLContext is not.
There was a problem hiding this comment.
Ah, I missed the comment stating that people should override it.
Problem is, to parse and interpret irdl dialects, I need the dependent (i.e., at least irdl?) dialexts to be available.
Our lazy loading mechanism is perfect for this, as it will only load actually needed dialects.
Now, without moving register_all_dialects, I would have to create an xDSLOptMain for this purpose, which can be wanted completely out of a tool's context.
I guess a compromise would be to keep a register_all_dialects method on xDSLOptMain, of which the default implementation is to just call MLContext's one? WDYT?
There was a problem hiding this comment.
I'm not 100% sure what the answer is to this but I would propose changing the mechanism in a different PR
Just some refactoring to decouple a few things.