-
Notifications
You must be signed in to change notification settings - Fork 54
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
Fix dictionary encoding and decoding #234
Fix dictionary encoding and decoding #234
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #234 +/- ##
==========================================
+ Coverage 92.39% 92.43% +0.04%
==========================================
Files 25 25
Lines 2815 2817 +2
==========================================
+ Hits 2601 2604 +3
+ Misses 214 213 -1
☔ View full report in Codecov by Sentry. |
Also fixed a typo in a comment
Thank you @JamieMair, well spotted. This seems to indeed be a bug in ProtoBuf.jl, the current approach to encoding/decoding As for this being a breaking change -- I think this qualifies as a bug and should be released as a patch version. I'll check downstream users on JuliaHub and open issues if necessary, giving them some time to adjust. Can I trouble you for writing a tests/tests for repeated messages and |
I agree, this additional overhead seems a bit redundant, but at least one can use arrays if that is an issue.
I can have a go when I next get some time to work on this. Just to check, would this involve something like
Is there something simpler I can do, or have I missed anything? |
Basically, yeah. I think you should be able to write a proto file with a I'll try to review the the rest of the PR soon. Again, thanks for your help! |
Great! I'll get on that when I next have a little time. Thanks for the quick response! |
…ector of PairStructs
@Drvi I have just found some time to add in the unit tests you asked for. Feel free to make any changes you want. Thanks for the support on this PR. |
@JamieMair Thanks a for the test! It made me realize that we were adding a length for the The JET failures on nightly are unrelated to this PR. |
I have just tried to test it now in our application. I have a test which checks the bytes generated (see https://github.com/JamieMair/TensorBoardLogger.jl/blob/update-to-new-protobuf/test/test_hparams.jl) but it seems that the encoded length byte on the object is wrong by 2. The second byte of the array should be 91 but it is encoding it as 93. I have tried to investigate why this is an issue, but I can't find it. I think possibly there is a problem with |
Ah, good catch @JamieMair I think I have a fix for that. |
@JamieMair Can you try again? |
@Drvi Yes, that's amazing. All works great on my end - I've removed our workaround. Thanks for finishing this one off! |
Fixes issues described in #233
Note that these would be breaking changes for any messages serialised which include dictionaries. Previously stored messages will fail when being deserialised. I am not 100% certain that these changes should be merged, but it is worth considering if the existing code did not match the existing specification. However, changing this may affect downstream users that are storing data which includes
Dict
types.