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

[releases/1.2] service: Fix tag deserialization for large messages #554

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

bkeryan
Copy link
Collaborator

@bkeryan bkeryan commented Dec 13, 2023

What does this Pull Request accomplish?

Cherry-pick #552 into releases/1.2.

Description:

Tags are i32 varints, but we were only deserializing the first byte.

Simplify some related details:

  • Decoding a varint returns the updated position, so we don't need to use encoder._TagSize()
  • We can directly construct a memory view once instead of constructing a BytesIO for each loop iteration.

Note

This reuses an internal+underscored protobuf function, google.protobuf.internal.decoder._DecodeSignedVarint32. This code was already using a different internal+underscored protobuf function, google.protobuf.internal.encoder._TagSize.

For xydata support, we forked the unsigned _DecodeVarint, but I didn't do that here.

In the long run, I think we should generate descriptors so that we can stop using protobuf internals and let protobuf do the heavy lifting: #35

Why should this Pull Request be merged?

Fixes AB#2602618: Protobuf Error on Measurement run [MeasurementLink UI Editor/InstrumentStudio]

Release the fix in version 1.2.1.

What testing has been done?

Ran poetry run pytest -v in both releases/1.2 and main branches.
Manually tested the service attached to the bug report with my users/bkeryan/fix-field-id-1.2 branch (targeting releases/1.2).

* tests: Add test cases for big messages (100 fields)

* service: Fix tag deserialization for large messages

Tags are i32 varints, but we were only deserializing the first byte.

Simplify some related details:
- Decoding a varint returns the updated position, so we don't need to use encoder._TagSize()
- We can directly construct a memory view once instead of constructing a BytesIO for each loop iteration.

* tests: Use pytest.approx instead of choosing exactly comparable floats
Copy link

Test Results

     12 files  ±  0       12 suites  ±0   1m 48s ⏱️ -5s
   199 tests +  2     169 ✔️ +  2    30 💤 ±0  0 ±0 
2 376 runs  +24  2 000 ✔️ +24  376 💤 ±0  0 ±0 

Results for commit e5d1379. ± Comparison against base commit 405f063.

@dixonjoel
Copy link
Collaborator

Are we also going to add a 1.2.0.1 bug fix release?

@bkeryan
Copy link
Collaborator Author

bkeryan commented Dec 13, 2023

Are we also going to add a 1.2.0.1 bug fix release?

Released Python packages have 3 version number fields to work with, so 1.2.1 would be the bug fix release. You can also do 1.2.0-post2 but that's typically for non-code changes such as packaging problems.

We can build a 1.2.1-dev0 for testing beforehand, though.

@bkeryan bkeryan merged commit 67f8b2a into releases/1.2 Dec 13, 2023
17 checks passed
@bkeryan bkeryan deleted the users/bkeryan/fix-field-id-releases-1.2 branch December 13, 2023 22:56
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