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

Remove placeholders #641

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

AdrienVannson
Copy link
Contributor

@AdrienVannson AdrienVannson commented Nov 12, 2024

Summary

Hello!

This is a first step in removing the PLACEHOLDER mechanism to greatly improve performance, make the whole code more Pythonic and simple and resolve a lot of bug. The motivation is more deeply described in #631

This PR completely removes placeholder values from messages. Instead, fields are always set with there default value (empty string, 0, None for optional, members of a oneof and messages, ...). Therefore, all the message field are marked as optional and can be set to None, which is coherent with the specification (see the issue).

As a consequence, this fixes the following issues:

Before merging it, I think it would be good to finish the work in progress (at least my other PR and another fix of the generation of the comments), and release a new version with all the recent bugfixes.

Performance

These changes also make it possible to remove the __getattribute__ from messages. As expected, this has a great impact on the performance.

With the following messages:

message Test {
    bool x = 1;
    int32 y = 2;
    float z = 3;
}

message List {
    repeated Test msg = 1;
}

The following code takes 3.24s on my computer with the original (master) implementation with the Rust codec, and only 2s with my PR (without the Rust codec). Thus, even if the Rust codec is not supported for now, it is not a problem since the performance are better anyway.

msg = List(msg=[Test(x=bool(i % 2), y = i, z = i / 2) for i in range(100000)])
b = bytes(msg)
msg2 = List().parse(b)
assert msg == msg2

Breaking changes

This PR introduces two small breaking changes:

  • Lazy-initialization of messages is no longer possible, since uninitialized message fields now have None as value and getattribute is no longer defined.
  • The serialized_on_wire function is now removed, since messages that were not serialized on the wire are now represented as None values. This function didn't really make sense in the first place imo, since being serialized is a property of a field of a message, not a property of a message itself (google's version has HasField, which indeed takes the field as a parameter).

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • This PR fixes an issue.
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)

@cetanu cetanu self-assigned this Nov 15, 2024
@cetanu cetanu requested review from Gobot1234 and cetanu November 15, 2024 08:17
Copy link
Collaborator

@Gobot1234 Gobot1234 left a comment

Choose a reason for hiding this comment

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

I do like this after more thought. Really do wish that PEP 505 happened

src/betterproto/__init__.py Outdated Show resolved Hide resolved
src/betterproto/__init__.py Outdated Show resolved Hide resolved
src/betterproto/__init__.py Outdated Show resolved Hide resolved
src/betterproto/__init__.py Outdated Show resolved Hide resolved
src/betterproto/__init__.py Outdated Show resolved Hide resolved
src/betterproto/__init__.py Outdated Show resolved Hide resolved
src/betterproto/__init__.py Outdated Show resolved Hide resolved
@AdrienVannson
Copy link
Contributor Author

Thanks! I applied your suggestions

@cetanu cetanu requested a review from Gobot1234 January 2, 2025 23:14
"group",
"oneof_index",
)
# TODO patch again to make field optional
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this no longer required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I let them for now as a reminder to change something else, but I'll remove it soon (in the fork)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(done)

@AdrienVannson
Copy link
Contributor Author

Note: I already merged this PR in the forked repo https://github.com/betterproto as discussed on Discord, so it's probably not worth it to solve the merge conflicts here

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.

3 participants