Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

OpType(Enum) falls down on python 3.11.1 #615

Merged
merged 3 commits into from
Dec 16, 2022
Merged

Conversation

tzaffi
Copy link
Contributor

@tzaffi tzaffi commented Dec 15, 2022

The Problem

There is a regression on python 3.11.1 which breaks the way we're using NamedTuples as an OpType(Enum) base type.
Even though it is expected that python 3.11.2 will fix the regression, there is a simple solution: swap in a dataclass instead.

Testing

I'm also introducing python 3.11 workflows to demonstrate the problem and ensure confidence in the fix. I propose that we keep these workflows in place to help detect future regressions. Please note that the example error occurred without any code changes, i.e. all I did was introduce the python 3.11.1 workflow. You can validate with this commit: df92197

@tzaffi tzaffi marked this pull request as ready for review December 16, 2022 00:26
@tzaffi tzaffi changed the title Op(Enum) should fall down on python 3.11.1 Op(Enum) falls down on python 3.11.1 Dec 16, 2022
@tzaffi tzaffi changed the title Op(Enum) falls down on python 3.11.1 OpType(Enum) falls down on python 3.11.1 Dec 16, 2022
Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

looks good, would be nice to add a line in CHANGELOG

@tzaffi
Copy link
Contributor Author

tzaffi commented Dec 16, 2022

looks good, would be nice to add a line in CHANGELOG

See the most recent commit for the CHANGELOG

@bbroder-algo
Copy link
Contributor

Hey this is my 5th minute looking at PyTeal and I can't really find any places that use the specific typing information that is/was provided by NamedTuple, but I did see some places that ask for the str() representation of a NamedTuple (for example, in tealop.py) and I verified that the str() for a dataclasses looks the same as a str() for NamedTuple).
`>>> from typing import NamedTuple

bob = NamedTuple ("BobType", [("value", str), ("mode", int)])
a = bob("Vlaue", 3)
from dataclasses import dataclass
@DataClass
... class BobDC:
... value:str
... mode:int
...
b = BobDC ("vvalue", "4")
print (str(b))
BobDC(value='vvalue', mode='4')
print (str(a))
BobType(value='Vlaue', mode=3)

`

Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for catching this

@tzaffi
Copy link
Contributor Author

tzaffi commented Dec 16, 2022

Hey this is my 5th minute looking at PyTeal and I can't really find any places that use the specific typing information that is/was provided by NamedTuple, but I did see some places that ask for the str() representation of a NamedTuple (for example, in tealop.py) and I verified that the str() for a dataclasses looks the same as a str() for NamedTuple). `>>> from typing import NamedTuple

bob = NamedTuple ("BobType", [("value", str), ("mode", int)])
a = bob("Vlaue", 3)
from dataclasses import dataclass
@DataClass
... class BobDC:
... value:str
... mode:int
...
b = BobDC ("vvalue", "4")
print (str(b))
BobDC(value='vvalue', mode='4')
print (str(a))
BobType(value='Vlaue', mode=3)

`

Thanks for the quick investigation. You add to my confidence that this change is safe. The main usages of OpType are:

  • switching logic (e.g. if op == Op.add: ...)
  • TEAL generation via stringifying (e.g. str(Op.add) )
  • and compatibility validation via the mode and min_version properties

All of these behaviors are remain the same under this proposed change, and all tests pass giving my high confidence.

@tzaffi tzaffi merged commit 6c1adac into master Dec 16, 2022
@tzaffi tzaffi deleted the py-3.11.1-readiness branch December 16, 2022 17:09
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants