Create double_val in oneof for program

[dstrain115]

• Creates an option to store double as an ArgValue for serialization.
• This is currently only one-way for backwards compatibility.
• The conversion of python floats will still go to float_value.

[balopat]

I assume this is to help with #3116 - but it doesn't really yet, right because we are not converting python floats to proto double? What's the plan/rationale for introducing this double field then?

[dstrain115]

I assume this is to help with #3116 - but it doesn't really yet, right because we are not converting python floats to proto double? What's the plan/rationale for introducing this double field then?

That's correct. We need to do this in two steps for backwards compatibility. We first introduce the field and the code to handle it.

Once this code is fully pushed out, then we can start converting python floats into the new double_val.

Make sense? I welcome alternatives if you have a different idea to preserve compatibility.

[balopat]

Yeah this is not possible with protobuf 3 gracefully. We are essentially changing the type of a field which is not backward compatible in this case.

Have we considered introducing a v3 api for this? Then we have the option to fully implement the full solution. Clients have the option to stay on v2 or switch over to v3 at their own leisure. I'm just worried about what "fully pushed out" means - how do we check with all clients that this has been updated?

[dstrain115]

Yeah this is not possible with protobuf 3 gracefully. We are essentially changing the type of a field which is not backward compatible in this case.

Upgrading to v3 is a way way way way bigger change. I don't think it is worth it for this change.
I mean, I think the plan would be to support deserializing here, then start serializing into this field in cirq 0.9.0.

The only thing we need to worry about is people deserializing programs with an old version, and I think that would be really unlikely, since clients would deserialize results, not programs. Only "servers" would need to deserialize programs, so we really only need to worry about QE and TFQ servers to upgrade here.

What do you think?

[balopat]

Yeah this is not possible with protobuf 3 gracefully. We are essentially changing the type of a field which is not backward compatible in this case.

Upgrading to v3 is a way way way way bigger change. I don't think it is worth it for this change.
I mean, I think the plan would be to support deserializing here, then start serializing into this field in cirq 0.9.0.

The only thing we need to worry about is people deserializing programs with an old version, and I think that would be really unlikely, since clients would deserialize results, not programs. Only "servers" would need to deserialize programs, so we really only need to worry about QE and TFQ servers to upgrade here.

What do you think?

That makes a lot of sense to me, thanks for explaining! LGTM!

[CirqBot]

Automerge cancelled: A status check is failing.