extra log files: remove legacy #5672

oliver-sanders · 2023-08-08T12:16:48Z

Closes extra log files: remove legacy #5610
Remove residual code leftover from the obsolete extra log files runtime configuration option.

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 if present).
Tests are included (or explain why tests are not needed).
CHANGES.md entry included if this is a change that can affect users
Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

oliver-sanders · 2023-08-08T12:18:56Z

cylc/flow/data_messages.proto

@@ -144,7 +144,6 @@ message PbJob {
    optional float execution_time_limit = 14;
    optional string platform = 15;
    optional string job_log_dir = 17;
-    repeated string extra_logs = 29;


@dwsutherland

This field is no longer needed, is it safe to remove a "repeated string"?

Could this cause compatibility problems between cylc-flow and cylc-uiserver at different versions?

I think this is fine, since I don't think we ask for this field anywhere.. And as long as we don't reuse 29..

MetRonnie · 2023-08-09T09:33:42Z

cylc/flow/data_messages_pb2.py

-  _globals['_ALLDELTAS']._serialized_start=8687
-  _globals['_ALLDELTAS']._serialized_end=8896
+  _PBMETA._serialized_start=34
+  _PBMETA._serialized_end=184


FYI it looks like you may have used an older version of protoc, if that matters. I tried the latest version 24.0 and it put back all of these _globals bits

Makes sense, I'll regenerate this based on the outcome of the above comment.

Is it actually sensible to generate this file with the latest version, should we be generating it with the oldest supported version?

I think it's safe to use within and up to the minor version, i.e. with pip freeze version:

protobuf==4.21.12

I used protoc version:

sutherlander@cortex:~$ protoc --version libprotoc 3.21.12

https://github.com/protocolbuffers/protobuf/releases/tag/v21.12

We should probably upgrade at some point lol .. might put up a PR after a few of these (including the no-rewalk) has gone in.

Can upgrade whenever, the only reason we pin this dependency so hard is that we haven't found out what the protobuf compatibility policy is yet, i.e. the span of versions should be inter-compatible.

After [a lot of] digging we were able to confirm that slackening the ZMQ dependency bounds was safe.

Frustrating lack of info on the compatibility of protoc versions - protocolbuffers/protobuf#11123

MetRonnie

Actually after looking at the docs a little, it seems we need to reserve the deleted field number: https://protobuf.dev/programming-guides/proto3/#deleting

dwsutherland

LGTM 👍

dwsutherland · 2023-09-07T02:39:34Z

Actually after looking at the docs a little, it seems we need to reserve the deleted field number: https://protobuf.dev/programming-guides/proto3/#deleting

As long as the UIS/UI/TUI never specifically asked for the field, I think we're fine.
The reason for for reserving the field is to not break clients (UIS) that were specifically asking for that field (which we just need to confirm we weren't (which I don't think we are

cylc-flow/cylc/flow/data_store_mgr.py

Line 289 in 5b76dba

def apply_delta(key, delta, data):

).

If the field is set/sent (in an older scheduler), it just won't be read into the new object definition on the UIS end.. Can always test this..

Load up new (this branch) version of UIS
Switch flow to not this branch, and run a workflow

UIS shouldn't break.

dwsutherland · 2023-09-07T08:22:11Z

Tested it,

UIS this branch (regenerated the data_messages_pb2.py)
Scheduler running master

and found working:

oliver-sanders · 2023-10-05T12:48:09Z

Right so, conclusions:

Compiling with an older protobuf version is fine and possibly safer.
Removing the field is ok so long as we were never using it.

oliver-sanders · 2023-10-05T13:19:06Z

I've tested this with the UIS/Scheduler running this-branch/8.2.1 and vice versa, it seems to be working fine.

MetRonnie · 2023-10-06T10:02:57Z

Thinking this through, I would imagine we still wouldn't want to be in the position where:

We've added a new field with this same field number
A new UIS asks for this field number from an old scheduler, thus getting the old extra_log_files result instead of whatever it was actually asking for

oliver-sanders · 2023-10-06T13:23:49Z

Enough to jam a comment in to "reserve" the old field number?

MetRonnie · 2023-10-06T15:36:00Z

I think all that's needed is

 message PbJob {
+  reserved 29;
   optional string stamp = 1;
   optional string id = 2;

https://protobuf.dev/programming-guides/proto3/#deleting,

* Closes cylc#5610 * Remove residual code leftover from the obsolete `extra log files` runtime configuration option.

oliver-sanders · 2023-10-11T16:00:50Z

Ok, reserved

MetRonnie · 2023-10-17T10:14:49Z

Did you regenerate the pb2 file after reserving the field?

oliver-sanders · 2023-10-17T12:04:58Z

Yes: https://github.com/cylc/cylc-flow/compare/abf2d031a4e0bbef1b8744bbe88c9e5bf2ee84ae..840096b2009e0c179e933b14218843f5ce17207e

MetRonnie

Testing with mixed versions of scheduler/UIS didn't bring up any problems

* upstream/master: speed up import time (cylc#5770) graphql: remove extraLogs field (cylc#5672) give a more useful error message if remote init fails (cylc#5720) clarify SLURM job handler docs (cylc#5748)

oliver-sanders added this to the cylc-8.3.0 milestone Aug 8, 2023

oliver-sanders self-assigned this Aug 8, 2023

oliver-sanders requested review from MetRonnie and dwsutherland August 8, 2023 12:17

oliver-sanders commented Aug 8, 2023

View reviewed changes

MetRonnie approved these changes Aug 9, 2023

View reviewed changes

MetRonnie requested changes Aug 17, 2023

View reviewed changes

dwsutherland approved these changes Sep 6, 2023

View reviewed changes

oliver-sanders requested a review from MetRonnie October 5, 2023 13:19

graphql: remove extraLogs field

840096b

* Closes cylc#5610 * Remove residual code leftover from the obsolete `extra log files` runtime configuration option.

oliver-sanders force-pushed the extra-logs branch from abf2d03 to 840096b Compare October 11, 2023 16:00

MetRonnie approved these changes Oct 17, 2023

View reviewed changes

MetRonnie merged commit 653317f into cylc:master Oct 17, 2023
21 of 23 checks passed

MetRonnie added the schema change Change to the Cylc GraphQL schema label Nov 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extra log files: remove legacy #5672

extra log files: remove legacy #5672

oliver-sanders commented Aug 8, 2023

oliver-sanders Aug 8, 2023

dwsutherland Sep 6, 2023

MetRonnie Aug 9, 2023

oliver-sanders Aug 9, 2023 •

edited

Loading

dwsutherland Sep 7, 2023

dwsutherland Sep 7, 2023

dwsutherland Sep 7, 2023

oliver-sanders Oct 5, 2023 •

edited

Loading

MetRonnie Oct 6, 2023 •

edited

Loading

MetRonnie left a comment

dwsutherland left a comment

dwsutherland commented Sep 7, 2023 •

edited

Loading

dwsutherland commented Sep 7, 2023 •

edited

Loading

oliver-sanders commented Oct 5, 2023

oliver-sanders commented Oct 5, 2023

MetRonnie commented Oct 6, 2023

oliver-sanders commented Oct 6, 2023

MetRonnie commented Oct 6, 2023

oliver-sanders commented Oct 11, 2023

MetRonnie commented Oct 17, 2023

oliver-sanders commented Oct 17, 2023

MetRonnie left a comment

extra log files: remove legacy #5672

extra log files: remove legacy #5672

Conversation

oliver-sanders commented Aug 8, 2023

oliver-sanders Aug 8, 2023

Choose a reason for hiding this comment

dwsutherland Sep 6, 2023

Choose a reason for hiding this comment

MetRonnie Aug 9, 2023

Choose a reason for hiding this comment

oliver-sanders Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

dwsutherland Sep 7, 2023

Choose a reason for hiding this comment

dwsutherland Sep 7, 2023

Choose a reason for hiding this comment

dwsutherland Sep 7, 2023

Choose a reason for hiding this comment

oliver-sanders Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

MetRonnie Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

MetRonnie left a comment

Choose a reason for hiding this comment

dwsutherland left a comment

Choose a reason for hiding this comment

dwsutherland commented Sep 7, 2023 • edited Loading

dwsutherland commented Sep 7, 2023 • edited Loading

oliver-sanders commented Oct 5, 2023

oliver-sanders commented Oct 5, 2023

MetRonnie commented Oct 6, 2023

oliver-sanders commented Oct 6, 2023

MetRonnie commented Oct 6, 2023

oliver-sanders commented Oct 11, 2023

MetRonnie commented Oct 17, 2023

oliver-sanders commented Oct 17, 2023

MetRonnie left a comment

Choose a reason for hiding this comment

oliver-sanders Aug 9, 2023 •

edited

Loading

oliver-sanders Oct 5, 2023 •

edited

Loading

MetRonnie Oct 6, 2023 •

edited

Loading

dwsutherland commented Sep 7, 2023 •

edited

Loading

dwsutherland commented Sep 7, 2023 •

edited

Loading