cylc-flow icon indicating copy to clipboard operation
cylc-flow copied to clipboard

protobuf major version update (4.21.1)

Open dwsutherland opened this issue 2 years ago • 8 comments

These changes close #4884

Requirements check-list

  • [x] I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • [x] Contains logically grouped changes (else tidy your branch by rebase).
  • [x] Does not contain off-topic changes (use other PRs for other changes).
  • [x] Applied any dependency changes to both setup.cfg and conda-environment.yml.
  • [x] Already covered by existing tests.
  • [x] No change log entry required (invisible to users).
  • [x] No documentation update required.

dwsutherland avatar May 28 '22 04:05 dwsutherland

Conda package needs to become available for 4.21.1

dwsutherland avatar May 28 '22 04:05 dwsutherland

Did you intend to remove cylc/flow/data_messages_pb2.py on this branch?

hjoliver avatar Jun 01 '22 02:06 hjoliver

Conda package needs to become available for 4.21.1

So this PR is blocked till then?

hjoliver avatar Jun 01 '22 02:06 hjoliver

Conda package needs to become available for 4.21.1

So this PR is blocked till then?

Yeah, I guess so:

Collecting package metadata (repodata.json): ...working... done
Solving environment: ...working... failed

ResolvePackageNotFound: 
  - protobuf[version='>=4.21.1']

I guess technically we could run with an older version of protobuf with the conda install ... but would be better to keep them inline.

dwsutherland avatar Jun 01 '22 03:06 dwsutherland

might want to wait till this one is resolved too: https://github.com/protocolbuffers/protobuf/issues/10088

Possibly a cause of some of our memory leaks (if they still exist)

dwsutherland avatar Jun 01 '22 03:06 dwsutherland

Did you intend to remove cylc/flow/data_messages_pb2.py on this branch?

I had to take a second look at that too .. It's not deleted, however, it has been drastically reduced (it's what the updated code generator spat out).. (and we are still importing our messages from there, i.e. PbWorkflow)

dwsutherland avatar Jun 01 '22 04:06 dwsutherland

I had to take a second look at that too .. It's not deleted, however, it has been drastically reduced (it's what the updated code generator spat out)..

A-ha, that's cool!

hjoliver avatar Jun 01 '22 04:06 hjoliver

Linked issue is still open so it's too late to get this in for 8.0.0. Will have to look into any inter-version compatibility issues later to ensure the UIS can work with older and newer versions side-by-side or work out a migration if not.

oliver-sanders avatar Jul 20 '22 15:07 oliver-sanders

v4.21.2 is out, unblocking this...

oliver-sanders avatar Sep 21 '22 12:09 oliver-sanders

v4.21.2 is out, unblocking this...

Not sure where you went to find this.. not here: https://anaconda.org/anaconda/protobuf

But, the install works 👍 .. PR updated

Tested flow --> uis --> ui .. apparently messages should be back compatible with 3.19.4

dwsutherland avatar Sep 28 '22 01:09 dwsutherland

Not sure where you went to find this.. not here: https://anaconda.org/anaconda/protobuf

Ah, the forge: https://anaconda.org/conda-forge/protobuf

dwsutherland avatar Sep 28 '22 01:09 dwsutherland

That macos test keeps failing/timing-out... the tests work for me (ubuntu)..

@oliver-sanders - could you check on your system? (since it's macos)

dwsutherland avatar Sep 28 '22 04:09 dwsutherland

I ran the tests which showed up as running after the timeout locally, they all passed quickly.

Running the full test battery locally to see if anything hangs but I think you just got unlucky, these timeouts do happen, from the timestamps the tests were running right up to the timeout so no indication of hanging.

As the number of functional tests continues to increase we will have to increase the timeout for Mac OS. Passionate plea from me to use integration tests more. Lots of functional tests don't actually need to be functional tests. I can run all doctests, unit and integration tests on my machine in just 20 seconds!

oliver-sanders avatar Sep 28 '22 10:09 oliver-sanders

BTW: I've checked and it looks like the UIS is able to handle the version change ok so we don't need to go bumping the API number to exclude 8.0 workflows from the 8.1 UIS (or vice versa).

This suggests we may be able to relax the protobuf version range but without a better understanding of protobuf compatibility I can't say.

oliver-sanders avatar Sep 30 '22 10:09 oliver-sanders