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

Add missing type_params attribute for ClassDef node before Python3.12 #89

Conversation

SigureMo
Copy link
Contributor

Add missing type_params attribute for ClassDef node before Python 3.12.

@serge-sans-paille
Copy link
Owner

Thanks! Could you add a test case?

gast/ast3.py Outdated
@@ -226,6 +237,18 @@ def visit_comprehension(self, node):
return ast.copy_location(new_node, node)

if 8 <= sys.version_info.minor < 12:

def visit_ClassDef(self, node):

Choose a reason for hiding this comment

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

if I'm not mistaken, it's not only for 8 <= sys.version_info.minor < 12 but just for sys.version_info.minor < 12, right?

otherwise looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the same code now exists in both 8 <= sys.version_info.minor < 12 and sys.version_info.minor < 8 branches, I'll put them together.

@serge-sans-paille serge-sans-paille merged commit b4d2df6 into serge-sans-paille:master Oct 23, 2024
9 checks passed
@serge-sans-paille
Copy link
Owner

thanks 🙇

@serge-sans-paille
Copy link
Owner

(btw in which context are you using gast ?)

@SigureMo SigureMo deleted the add-missing-type-params-attr-for-class-def branch October 23, 2024 12:43
@SigureMo
Copy link
Contributor Author

SigureMo commented Oct 23, 2024

(btw in which context are you using gast ?)

I'm currently maintaining PaddlePaddle, a deep learning framework similar to PyTorch, and we need to rewrite the original dynamic graph functions at the source level as static graph functions with control flow operators. To ensure multi-version compatibility, we use gast as a unified intermediate representation in the JIT module.

Note

For some historical reason unknown to me, the source code was copied to the PaddlePaddle repo and then used. This problem was also exposed after the upgrade, and I fixed it at PaddlePaddle (see PaddlePaddle/Paddle#68892). In order to prevent others from encountering the same problem, I thought it would be a good idea to fix it in gast as well, so I've open this PR.

Thanks for writing the gast, it was very helpful!

@serge-sans-paille
Copy link
Owner

Cool! Thanks for sharing :-) It would indeed be better not to vendor gast, should I submit a PR that fixes this in PaddlePaddle?

@SigureMo
Copy link
Contributor Author

should I submit a PR that fixes this in PaddlePaddle?

Thanks, this issue has been fixed at PaddlePaddle/Paddle#68892.

If we encounter similar issues in the future, we will also fix them and contribute them back to gast.

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.

2 participants