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.

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

xdsl: Use type annotations in PyRDL #338

Merged
merged 8 commits into from Jan 24, 2023
Merged

xdsl: Use type annotations in PyRDL #338

merged 8 commits into from Jan 24, 2023

Conversation

math-fehr
Copy link
Collaborator

This PR updates PyRDL to use type annotations.
This should greatly reduce the amount of PyRight type checking errors.

@codecov
Copy link

codecov bot commented Jan 13, 2023

Codecov Report

Base: 88.53% // Head: 88.61% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (9cd48a2) compared to base (b09e94e).
Patch coverage: 94.52% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #338      +/-   ##
==========================================
+ Coverage   88.53%   88.61%   +0.07%     
==========================================
  Files          65       65              
  Lines        8017     8054      +37     
  Branches     1270     1278       +8     
==========================================
+ Hits         7098     7137      +39     
+ Misses        660      658       -2     
  Partials      259      259              
Impacted Files Coverage Δ
xdsl/frontend/symref.py 0.00% <0.00%> (ø)
xdsl/irdl.py 86.00% <94.44%> (+1.17%) ⬆️
tests/test_irdl.py 96.29% <100.00%> (ø)
tests/test_mlir_printer.py 100.00% <100.00%> (ø)
tests/test_operation_builder.py 100.00% <100.00%> (ø)
tests/test_operation_definition.py 100.00% <100.00%> (ø)
tests/test_ssavalue.py 100.00% <100.00%> (ø)
xdsl/dialects/affine.py 100.00% <100.00%> (ø)
xdsl/dialects/arith.py 94.63% <100.00%> (ø)
xdsl/dialects/builtin.py 82.57% <100.00%> (ø)
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@math-fehr math-fehr marked this pull request as ready for review January 16, 2023 00:51
@math-fehr
Copy link
Collaborator Author

Note that this doesn't remove as much PyRight errors as we thought, since the main Pyright errors are from operands and results.

@math-fehr
Copy link
Collaborator Author

I changed the names for attribute definitions, since we cannot use AttributeDef anymore.
I renamed it OpAttr, to follow OpResult, though I agree this is poor naming.
I don't have a good option in mind though

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.

LGTM!

xdsl/irdl.py Show resolved Hide resolved
@superlopuh superlopuh added this to the Convolve 2023-02 milestone Jan 19, 2023
xdsl/irdl.py Outdated Show resolved Hide resolved
xdsl/irdl.py Show resolved Hide resolved
Copy link
Collaborator

@webmiche webmiche left a comment

Choose a reason for hiding this comment

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

LGTM!

I feel like an example that now works would be cool, but I guess that is mainly visible in the pyright CI as well as the IDE support.

tests/test_operation_builder.py Show resolved Hide resolved
tests/test_operation_definition.py Show resolved Hide resolved
xdsl/dialects/memref.py Show resolved Hide resolved
xdsl/dialects/scf.py Show resolved Hide resolved
@@ -448,6 +469,13 @@ def __init__(self, typ: Attribute | type[Attribute] | AttrConstraint):
super().__init__(typ)


_OpAttrT = TypeVar("_OpAttrT", bound=Attribute)

OpAttr: TypeAlias = Annotated[_OpAttrT, IRDLAnnotations.AttributeDefAnnot]
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to think about this naming.

  1. it sounds bad.
  2. do we want attributes that can only be attached to ops (OpAttr sounds like it).

Maybe AttrAnnotation, Attribute, AttributeAnnot. This is very hard and I feel my names are bad aswell 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So the annotations should be private for the users, and are just used for introspection.
We sadly cannot use Attribute, because this class has to be different than Attribute.
I went with OpAttr since we have OpResult, but yeah that's ugly especially with OptOpAttr.

xdsl/irdl.py Show resolved Hide resolved
@superlopuh
Copy link
Member

It would be nice to get this in as I rely on some of the bug fixes in this PR for Chapter 2 of the tutorial. Would it be ok if I addressed the comments on this PR while Mathieu is on holiday?

@math-fehr
Copy link
Collaborator Author

I think all comments should be addressed now, besides the naming one which no one seems to have a good idea :/

@math-fehr
Copy link
Collaborator Author

I'll let you decide what to do with this PR, but for me I'm fine to merge it now, and maybe think more of better naming for later?

@superlopuh
Copy link
Member

We have Find/Replace for naming, I think the type names are fine :)

@webmiche
Copy link
Collaborator

Yes sure, let's merge it and fix the naming another day.

@webmiche webmiche merged commit 709d492 into main Jan 24, 2023
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.

None yet

4 participants