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.

Sign up for GitHub

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

Jump to bottom

core: Parse module correctly when allow-unregistered-dialect is set #3669

Merged
merged 4 commits into from
Dec 23, 2024
Merged

core: Parse module correctly when allow-unregistered-dialect is set #3669

merged 4 commits into from
Dec 23, 2024

Conversation

alexarice
Copy link
Collaborator

Fixes #3668

@alexarice alexarice added bug Something isn't working core xDSL core (ir, textual format, ...) labels Dec 22, 2024
@alexarice alexarice self-assigned this Dec 22, 2024
dshaaban01
dshaaban01 approved these changes Dec 22, 2024
View reviewed changes
Copy link
Collaborator

@dshaaban01 dshaaban01 left a comment

Choose a reason for hiding this comment

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

Amazing🥳 thank you!

@codecov Codecov
Copy link

codecov bot commented Dec 22, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.29%. Comparing base (42681e6) to head (f4c6c4d).
Report is 2 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #3669   +/-   ##
=======================================
  Coverage   91.29%   91.29%           
=======================================
  Files         466      466           
  Lines       58332    58354   +22     
  Branches     5638     5639    +1     
=======================================
+ Hits        53253    53276   +23     
  Misses       3629     3629           
+ Partials     1450     1449    -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

superlopuh
superlopuh reviewed Dec 23, 2024
View reviewed changes
Copy link
Member

@superlopuh superlopuh left a comment

Choose a reason for hiding this comment

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

This LGTM, but I'm wondering whether we could simplify things by passing the dialect stack to get_optional_op directly, avoiding the creation of extra ops?

@alexarice
Copy link
Collaborator Author

This LGTM, but I'm wondering whether we could simplify things by passing the dialect stack to get_optional_op directly, avoiding the creation of extra ops?

Sounds good, I knew there was something I didn't like about my solution

@alexarice
Copy link
Collaborator Author

I've rewritten it with @superlopuh suggestion. This didn't quite fix the needless creation of unregistered ops so I've moved some things to a helper function. Also added 2 unit tests.

@alexarice alexarice requested a review from superlopuh December 23, 2024 11:23
math-fehr
math-fehr approved these changes Dec 23, 2024
View reviewed changes
Copy link
Collaborator

@math-fehr math-fehr left a comment

Choose a reason for hiding this comment

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

Nice!

@alexarice alexarice merged commit fb41de5 into main Dec 23, 2024
16 checks passed
@alexarice alexarice deleted the alexarice/parse_unregistered_module branch December 23, 2024 19:36
superlopuh
superlopuh reviewed Dec 24, 2024
View reviewed changes
tests/test_mlcontext.py

assert ctx.get_optional_op("dummy", dialect_stack=("test",)) == DummyOp
op_type = ctx.get_optional_op("dummy2", dialect_stack=("test",))
print(op_type)
Copy link
Member

Choose a reason for hiding this comment

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

Could you please open a mini PR removing this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#3673

Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@superlopuh superlopuh superlopuh left review comments

@dshaaban01 dshaaban01 dshaaban01 approved these changes

@math-fehr math-fehr math-fehr approved these changes

Assignees

@alexarice alexarice

Labels
bug Something isn't working core xDSL core (ir, textual format, ...)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

core: module{} parsed as an unregistered operation when MLContext(allow_unregistered=True)
4 participants
@alexarice @superlopuh @math-fehr @dshaaban01