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

protobuf major version update (4.21.*) #4901

Merged
merged 1 commit into from
Sep 30, 2022

Conversation

dwsutherland
Copy link
Member

These changes close #4884

Requirements check-list

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

@dwsutherland dwsutherland added this to the cylc-8.0rc4 milestone May 28, 2022
@dwsutherland dwsutherland self-assigned this May 28, 2022
@dwsutherland dwsutherland changed the title protobuf major version update (4.21.0) protobuf major version update (4.21.1) May 28, 2022
@dwsutherland dwsutherland force-pushed the protobuf-4.21.0-update branch from 212c661 to 02a7b1e Compare May 28, 2022 04:25
@dwsutherland
Copy link
Member Author

Conda package needs to become available for 4.21.1

@hjoliver
Copy link
Member

hjoliver commented Jun 1, 2022

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

@hjoliver
Copy link
Member

hjoliver commented Jun 1, 2022

Conda package needs to become available for 4.21.1

So this PR is blocked till then?

@hjoliver hjoliver added the BLOCKED This can't happen until something else does label Jun 1, 2022
@dwsutherland
Copy link
Member Author

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
Copy link
Member Author

dwsutherland commented Jun 1, 2022

might want to wait till this one is resolved too:
protocolbuffers/protobuf#10088

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

@dwsutherland
Copy link
Member Author

dwsutherland commented Jun 1, 2022

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)

@hjoliver
Copy link
Member

hjoliver commented Jun 1, 2022

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 hjoliver modified the milestones: cylc-8.0rc4, cylc-8.0.0 Jul 5, 2022
@oliver-sanders oliver-sanders modified the milestones: cylc-8.0.0, cylc-8.x Jul 20, 2022
@oliver-sanders
Copy link
Member

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
Copy link
Member

v4.21.2 is out, unblocking this...

@oliver-sanders oliver-sanders removed the BLOCKED This can't happen until something else does label Sep 21, 2022
@dwsutherland
Copy link
Member Author

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
Copy link
Member Author

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 dwsutherland changed the title protobuf major version update (4.21.1) protobuf major version update (4.21.*) Sep 28, 2022
@dwsutherland
Copy link
Member Author

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)

@oliver-sanders
Copy link
Member

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!

@dwsutherland dwsutherland force-pushed the protobuf-4.21.0-update branch 2 times, most recently from 3384fde to 92b26d0 Compare September 28, 2022 19:17
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me.

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one comment to resolve then ready to merge.

setup.cfg Outdated Show resolved Hide resolved
@oliver-sanders oliver-sanders modified the milestones: cylc-8.x, cylc-8.1.0 Sep 30, 2022
@oliver-sanders oliver-sanders merged commit 5f2583f into cylc:master Sep 30, 2022
@oliver-sanders
Copy link
Member

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.

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

Successfully merging this pull request may close these issues.

deps: bump protobuf
3 participants