Cylc-flow: Update Protobuf to 3.12+ - Fields with default value not set in deltas

Created on 4 May 2020  Â·  3Comments  Â·  Source: cylc/cylc-flow

Problem:
(from Riot)

Scalar fields like bool, int, string (...etc) have default values (False, 0, “”), however , at the receiving end you cannot tell whether a field was intentionally set to the default...

For example:

>>> from cylc.flow.data_messages_pb2 import PbWorkflow
>>> var1 = PbWorkflow(id='foo', host='', port=0)
>>> var1.ListFields()
[(<google.protobuf.pyext._message.FieldDescriptor object at 0x7fa3ac80edd0>, 'foo')]
>>> var1
id: "foo"

I posted my frustrations in a long standing issue here:
https://github.com/protocolbuffers/protobuf/issues/359
(This changed with proto2 => proto3)

This means things like latestMessage, isHeldTotal, port (etc..) have to either always send the data value from UIS data-store or only send the non-default... Unacceptable, as there’s no option in proto3 to have alternate defaults...

I/we don’t want clunky workarounds..

Solution(s):

1) A developer from Protobuf responded (fairly quickly);
https://github.com/protocolbuffers/protobuf/issues/359#issuecomment-623050883
and referenced an upcoming release feature to fix this:
https://github.com/protocolbuffers/protobuf/blob/master/docs/field_presence.md
(which looks like it's in master)

syntax = "proto3";
package example;

message MyMessage {
  // No presence:
  int32 not_tracked = 1;

  // Explicit presence:
  optional int32 tracked = 2;
}

With the protoc run with option --experimental_allow_proto3_optional..
This is suppose to be available via release from 3.12 or later..

2) I don’t think it’ll be hard to change (heck we could probably even pickle GraphQL ObjectTypes if we wanted or just use dict)..

This issue came to light when working with the delta subscription PRs:
https://github.com/cylc/cylc-flow/pull/3500
https://github.com/cylc/cylc-uiserver/pull/118
And can be addressed at any point with option 1) (and the availability of protobuf=3.12.*), however anything more drastic should wait until after they go in.

Pull requests welcome!
This is an Open Source project - please consider contributing a bug fix
yourself (please read CONTRIBUTING.md before starting any work though).

bug

Most helpful comment

New release out:
https://github.com/protocolbuffers/protobuf/releases/tag/v3.12.0-rc1

Tested and solves this issue with fields as optional.. However I did report an issue:
https://github.com/protocolbuffers/protobuf/issues/7463

All 3 comments

I would vote for option 1), and wait for the relevant Protobuf "fix"..

Then if we come across any other major roadblocks/undesirables, investigate 2).

New release out:
https://github.com/protocolbuffers/protobuf/releases/tag/v3.12.0-rc1

Tested and solves this issue with fields as optional.. However I did report an issue:
https://github.com/protocolbuffers/protobuf/issues/7463

Tested and solves this issue with fields as optional.. However I did report an issue:

I think they should be quite quick fixing this one too. :crossed_fingers:

Was this page helpful?
0 / 5 - 0 ratings

Related issues

hjoliver picture hjoliver  Â·  5Comments

kinow picture kinow  Â·  4Comments

dpmatthews picture dpmatthews  Â·  3Comments

oliver-sanders picture oliver-sanders  Â·  3Comments

sadielbartholomew picture sadielbartholomew  Â·  4Comments