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

deps: bump protobuf #4884

Closed
oliver-sanders opened this issue May 19, 2022 · 5 comments · Fixed by #4901
Closed

deps: bump protobuf #4884

oliver-sanders opened this issue May 19, 2022 · 5 comments · Fixed by #4901

Comments

@oliver-sanders
Copy link
Member

From element (@dwsutherland)

Protobuf new release claims significant performance boost: https://developers.google.com/protocol-buffers/docs/news/2022-05-06#python-updates

just a release candidate at the moment (I think)
Changes made in May, 2022 | Protocol Buffers | Google Developers - Google Developers
Protocol Buffers Language English Bahasa Indonesia Deutsch Español Français Português – Brasil Русский
might be some jupyter that uses protobuf .. so will need to check if any conflicts (or maybe that was just zmq between hub and server)

Look into bumping the protobuf release, be careful to avoid creating version conflicts with Jupyter projects.

Note the reason for our strict version dependence is that as a distributed system different Cylc installations need to be compatible. Look into protobuf back compat support to determine how strict/loose we need to be when pinning protobuf.

@dwsutherland
Copy link
Member

@dwsutherland
Copy link
Member

dwsutherland commented May 27, 2022

No dependence of Jupyter Server on protobuf:

requires-python = ">=3.7"
dependencies = [
    "anyio>=3.1.0,<4",
    "argon2-cffi",
    "jinja2",
    "jupyter_client>=6.1.12",
    "jupyter_core>=4.7.0",
    "jupyter_server_terminals",
    "nbconvert>=6.4.4",
    "nbformat>=5.2.0",
    "packaging",
    "prometheus_client",
    "pywinpty;os_name=='nt'",
    "pyzmq>=17",
    "Send2Trash",
    "terminado>=0.8.3",
    "tornado>=6.1.0",
    "traitlets>=5.1",
    "websocket-client",
    "jupyter_telemetry"
]

https://github.com/jupyter-server/jupyter_server/blob/main/pyproject.toml

They just use json to serialize and deserialize messages:
https://github.com/jupyter-server/jupyter_server/blob/main/jupyter_server/base/zmqhandlers.py

So we should be safe.

Also:
Python upb requires generated code that has been generated from protoc 3.19.0 or newer.

We are at 3.19.* so we could upgrade without regenerating code from our data_messages.proto (if our code is at that version)..
(although, I would be inclined to keep our generated code up-to-date)

@dwsutherland
Copy link
Member

I found a bug in their new release..
Using the following message case:

message PbTestNode {
    optional PbTestEdges edges = 1;
}

message PbTestEdges {
    optional string id = 1;
    repeated string leaves = 2;
}

When running with protobuf==4.21.0 (and our code generated to match) I come across this:

(flow) sutherlander@graphic-vbox:cylc-flow$ python
Python 3.8.10 (default, Mar 15 2022, 12:22:08) 
[GCC 9.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from google.protobuf.internal import api_implementation
>>> print(api_implementation.Type())
upb
>>> from cylc.flow.data_messages_pb2 import PbTestNode
>>> n1 = PbTestNode()
>>> n1.edges.id = 'flint'
>>> n1.edges.leaves[:] = ['one', 'two', 'three']
>>> n2 = PbTestNode()
>>> n2.edges.leaves[:] = ['one', 'two', 'three']
Segmentation fault (core dumped)

Which doesn't happen with the previous version:

Successfully installed cylc-flow-8.0rc4.dev0 protobuf-3.20.1
(flow) sutherlander@graphic-vbox:cylc-flow$ python
Python 3.8.10 (default, Mar 15 2022, 12:22:08) 
[GCC 9.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from google.protobuf.internal import api_implementation
>>> print(api_implementation.Type())
cpp
>>> from cylc.flow.data_messages_pb2 import PbTestNode
>>> n3 = PbTestNode()
>>> n3.edges.leaves[:] = ['one', 'two', 'three']
>>> exit()

Will open a ticket their end..

@dwsutherland
Copy link
Member

Issue here:
protocolbuffers/protobuf#10063

@dwsutherland dwsutherland self-assigned this May 27, 2022
@dwsutherland
Copy link
Member

Fixed:
https://github.com/protocolbuffers/protobuf/releases/tag/v21.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants