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

Do not use class variables in runtime modules #642

Merged
merged 2 commits into from
Jul 19, 2024

Conversation

wwwillchen
Copy link
Collaborator

@wwwillchen wwwillchen commented Jul 19, 2024

  • It's better to do the type annotations inside __init__ instead of annotating at the class variable.
  • Initializing class variables is sketchy, particularly with mutable objects.
  • We were previously initializing class variables to mutable values on Runtime, which is technically not too bad because Runtime is a singleton object, but this is still a bad practice.

Even though Runtime is a singleton object, it's still not a good practice.
@wwwillchen wwwillchen changed the title Do not initialize mutable objects at the class-level Do not use class variables Jul 19, 2024
Copy link
Collaborator

@richard-to richard-to left a comment

Choose a reason for hiding this comment

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

Oh wow, that's a great catch. Can't believe I never noticed that.

@wwwillchen
Copy link
Collaborator Author

wwwillchen commented Jul 19, 2024

Oh wow, that's a great catch. Can't believe I never noticed that.

yeah, I was a little shocked and then I realized it's because dataclass purposely blurs the line between between class variables and instance variables (e.g. you define it as a class variable but you use it like an instance variable) so it's easy to get this mixed up :(

@wwwillchen wwwillchen changed the title Do not use class variables Do not use class variables in runtime modules Jul 19, 2024
@wwwillchen wwwillchen merged commit afa304b into google:main Jul 19, 2024
3 checks passed
@wwwillchen wwwillchen deleted the safe_init branch July 19, 2024 23:34
@wwwillchen wwwillchen mentioned this pull request Jul 20, 2024
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