FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Use a custom Enum class for improved speed by Gobot1234 · Pull Request #117 · danielgtaylor/python-betterproto · GitHub

/ python-betterproto Public

Use a custom Enum class for improved speed - #117

Closed
Gobot1234 wants to merge 34 commits into
danielgtaylor:masterfrom
Gobot1234:enums
ConversationCommits (34)ChecksFiles changed
Closed

Use a custom Enum class for improved speed#117
Gobot1234 wants to merge 34 commits into
danielgtaylor:masterfrom
Gobot1234:enums

Conversation

Gobot1234 commented Jul 11, 2020

Copy link
Copy Markdown
Collaborator

Significant speed improvements over the builtin Enum class.

  • 3x faster at creating instances of the enums.
  • Roughly 2x as fast at creating small (15 member) enums.
  • About 3x times faster at creating larger (50 member) enums.
  • About 7x times faster direct attribute access, then 9x faster for name/values.

Full tests here https://gist.github.com/Gobot1234/e3883e9db151ce3b33adf4d38c140936

Fixes a bug discussed on slack relating to to_dict and IndexErrors when using Enums.

boukeversteegh commented Jul 12, 2020

Copy link
Copy Markdown
Collaborator

Hi Gobot, thank you for your efforts.
For the bug, there is already an open pull request: #102

boukeversteegh requested changes Jul 12, 2020
View reviewed changes

boukeversteegh left a comment •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you for helping out.
As you know I'm not much convinced to the practical benefits of faster Enums.

If you could cover the new Enum class with unit tests, so it is clear what features are supported, I would be confident enough that it wouldn't break anything so that we can merge it nonetheless.

My two concerns are

  • extra mental load on maintainers and users who, in the course of encountering or fixing bugs (of which we have plenty still, also related to enums), will consider the Enum class as a potential root cause, and spend extra time going through and debugging the Enum code.
    We can avoid this by guaranteeing that our custom Enum implementation behaves 100% the same as the Python one. Full test coverage seems essential in my opinion.
  • Incompatibility with how users were applying betterproto Enums before. Again tests can rule this out to some extent. I did a quick search to see how SQL Alchemy uses Enums for example, but it turns out they don't natively support it, and the same goes for Django forms. Seems that Enum support is not widespread so that lowers concerns for compatibility with other libraries.

For tests, we can probably reuse most tests from the cpython:

https://github.com/python/cpython/blob/6a1e9c26736259413b060b01d1cb52fcf82c0385/Lib/test/test_enum.py#L140

Comment thread betterproto/enums.py Outdated
Comment thread betterproto/enums.py Outdated
Comment thread betterproto/__init__.py

def to_dict(
self, casing: Casing = Casing.CAMEL, include_default_values: bool = False
self, casing: Optional[Casing] = None, include_default_values: bool = False

boukeversteegh Jul 12, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Since Enums are not mutable, do we need this change? Or was passing a default not possible with the new class?

I think it was helpful for developers to see the default casing from the parameter.

Gobot1234 Jul 12, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Unfortunately it raises an AttributeError as the class hasn't been created yet.

boukeversteegh Jul 12, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Actually, I guess we don't need to use Betterproto Enums for Casing. Perhaps just keep things simple and use the python Enum instead?

Gobot1234 Jul 13, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't think it's that big of a deal, if someone wants to see the default they can just view source.

Comment thread betterproto/__init__.py Outdated
boukeversteegh reviewed Jul 12, 2020
View reviewed changes
Comment thread betterproto/enums.py Outdated

Gobot1234 commented Jul 12, 2020

Copy link
Copy Markdown
Collaborator Author

Everything should now work

Gobot1234 commented Jul 13, 2020

Copy link
Copy Markdown
Collaborator Author

This is a bad idea, making these consistent is too much of a pain, sticking with the current implementation is fine.

Gobot1234 closed this Jul 13, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

boukeversteegh boukeversteegh requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL