chore: upgrade protoc used in CI #7935

hamidzr · 2023-09-19T20:57:11Z

Description

protoc was upgraded a while back and the new version is not compatible with the old.
make -C proto check started fails locally when using the new version. This updates CI to use a newer version (24)
https://github.com/protocolbuffers/protobuf/releases

Test Plan

hbp ➜ d/determined git:(upgrade-protoc) ✗ make -C proto dep_check
make: Entering directory '/Users/hmd/projects/da/determined/proto'
echo "libprotoc 3.24.54" | tee /dev/stderr | \
	perl -ne '(/libprotoc (?:3?\.)?(\d+)/ && $1>=24) || \
		(print("Please install protoc version >= 24\n") && exit 1)'
libprotoc 3.24.54
make: Leaving directory '/Users/hmd/projects/da/determined/proto'
hbp ➜ d/determined git:(upgrade-protoc) ✗ make -C proto dep_check
make: Entering directory '/Users/hmd/projects/da/determined/proto'
echo "libprotoc 3.23.54" | tee /dev/stderr | \
	perl -ne '(/libprotoc (?:3?\.)?(\d+)/ && $1>=24) || \
		(print("Please install protoc version >= 24\n") && exit 1)'
libprotoc 3.23.54
Please install protoc version >= 24
make: *** [Makefile:37: dep_check] Error 1
make: Leaving directory '/Users/hmd/projects/da/determined/proto'
hbp ➜ d/determined git:(upgrade-protoc) ✗ make -C proto dep_check
make: Entering directory '/Users/hmd/projects/da/determined/proto'
echo "libprotoc 23.54" | tee /dev/stderr | \
	perl -ne '(/libprotoc (?:3?\.)?(\d+)/ && $1>=24) || \
		(print("Please install protoc version >= 24\n") && exit 1)'
libprotoc 23.54
Please install protoc version >= 24
make: *** [Makefile:37: dep_check] Error 1
make: Leaving directory '/Users/hmd/projects/da/determined/proto'
hbp ➜ d/determined git:(upgrade-protoc) ✗ make -C proto dep_check
make: Entering directory '/Users/hmd/projects/da/determined/proto'
echo "libprotoc 24.54" | tee /dev/stderr | \
	perl -ne '(/libprotoc (?:3?\.)?(\d+)/ && $1>=24) || \
		(print("Please install protoc version >= 24\n") && exit 1)'
libprotoc 24.54
make: Leaving directory '/Users/hmd/projects/da/determined/proto'
hbp ➜ d/determined git:(upgrade-protoc) ✗

Commentary (optional)

https://hpe-aiatscale.slack.com/archives/C02PV33GSN5/p1712089871140569

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

netlify · 2023-09-19T20:57:16Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`aaae2e1`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/6627c0d15df456000889fb12
😎 Deploy Preview	https://deploy-preview-7935--determined-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

dzhu · 2023-11-27T18:13:48Z

Will this require everyone to upgrade their local version?

hamidzr · 2023-11-28T02:28:08Z

Will this require everyone to upgrade their local version?

yes. We had a talk about this in the backend team around when it's okay to require dependency upgrades and the consensus was that as long as it's communicated we're good with it and we didn't go into more details. What do you think

dzhu

Sure, just checking that it was considered at some point.

hamidzr · 2023-12-04T19:41:17Z

I want to update

determined/CONTRIBUTING.md

Line 58 in ed7a283

- protobuf-compiler (>= 3.15)

but I haven't been able to pin down which specific version introduces the material change.

hamidzr · 2024-04-02T20:21:13Z

@varlogtim had set a cutoff here at >v22 so maybe we could expand it to that but since we're testing 24.3 with the ci and it's been out for a while let's just go for that?
https://github.com/protocolbuffers/protobuf/releases/tag/v24.3

codecov · 2024-04-02T20:24:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.70%. Comparing base (9f6bbc9) to head (aaae2e1).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7935   +/-   ##
=======================================
  Coverage   44.70%   44.70%           
=======================================
  Files        1270     1270           
  Lines      155132   155132           
  Branches     2437     2437           
=======================================
+ Hits        69351    69352    +1     
+ Misses      85545    85544    -1     
  Partials      236      236

Flag	Coverage Δ
backend	`41.57% <ø> (+<0.01%)`	⬆️
harness	`64.31% <ø> (ø)`
web	`35.23% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

see 2 files with indirect coverage changes

erikwilson · 2024-04-02T21:37:45Z

proto/Makefile

@@ -35,8 +35,8 @@ get-deps:

 build/proto.stamp: $(source_files)
 	protoc --version | tee /dev/stderr | \
-		perl -ne '(/libprotoc (?:3?\.)?(\d+)\.(?:\d+)/ && $$1>=15 && $$1<22) || \
-			(print("Please install protoc where version >= 3.15 and < 3.22\n") && exit 1)'
+		perl -ne '(/libprotoc (?:24\.)?(\d+)\.(\d+)/ && $$1>=24 && ($$1>24 || $$2>=3)) || \


It looks like this regex change may not be doing what is intended. At some point protoc dropped the major version from the version number (altho v3 tags are still being created). The ?: part of the regex indicates a non-capturing* group, so the major part should not change it is just the minor and patch versions being assigned to $1 and $2. The max minor version check was dropped, does protobuf v26 also work here? edit: It also seems like we should not care about checking the patch version.

More info on the versioning change:
https://protobuf.dev/news/2022-05-06/, basically the major version is reserved for language specific api updates, but previously protoc would report the major version as 3. It should probably stay as 3 or >=3.

We should also make sure whatever minor version is easily installable for devs. Right now we can brew install protobuf@21, and brew install protobuf currently installs v26.

I'm thinking to leave it a >21 to keep it simple and the min version similar to ci?

personally I would prefer stability to running the latest thing, unless there is a particular reason we need to upgrade. eventually something will be mismatched between versions, causing breaking CI issues again.

made sure we keep allowing > 3.24.x

hamidzr · 2024-04-16T17:27:27Z

makes sense to wait for the unforking to happen before landing this

and has been out for a while

This reverts commit 26c1940.

cla-bot bot added the cla-signed label Sep 19, 2023

hamidzr marked this pull request as draft September 19, 2023 21:03

carolinaecalderon force-pushed the main branch from 9ac8f82 to 8eff8ac Compare September 20, 2023 15:14

hamidzr force-pushed the upgrade-protoc branch from b359416 to a9a768b Compare November 22, 2023 23:53

hamidzr marked this pull request as ready for review November 23, 2023 01:08

hamidzr requested review from a team as code owners November 23, 2023 01:08

hamidzr requested review from dzhu and salonig23 November 23, 2023 01:08

dzhu approved these changes Nov 30, 2023

View reviewed changes

dzhu assigned hamidzr Nov 30, 2023

hamidzr enabled auto-merge (squash) December 4, 2023 19:34

hamidzr disabled auto-merge December 4, 2023 19:35

salonig23 approved these changes Dec 4, 2023

View reviewed changes

hamidzr force-pushed the upgrade-protoc branch from ed7a283 to ec1e0f0 Compare April 2, 2024 20:20

erikwilson reviewed Apr 2, 2024

View reviewed changes

hamidzr force-pushed the upgrade-protoc branch 2 times, most recently from 076654f to e3dc47b Compare April 16, 2024 17:20

hamidzr added 5 commits April 23, 2024 09:07

upgrade CI's protoc to v24

8150b6a

not sure the exact min version but this works fine

c452380

and has been out for a while

add the version check for protoc

8af2ca5

regen

9b66d0f

add the inital 3 ignore back; make a dep check

aaae2e1

hamidzr force-pushed the upgrade-protoc branch from e3dc47b to aaae2e1 Compare April 23, 2024 14:08

hamidzr merged commit 26c1940 into main Apr 23, 2024
86 of 98 checks passed

hamidzr deleted the upgrade-protoc branch April 23, 2024 15:06

hamidzr added a commit that referenced this pull request Apr 24, 2024

Revert "chore: upgrade protoc used in CI (#7935)"

042a2bf

This reverts commit 26c1940.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: upgrade protoc used in CI #7935

chore: upgrade protoc used in CI #7935

hamidzr commented Sep 19, 2023 •

edited

Loading

netlify bot commented Sep 19, 2023 •

edited

Loading

dzhu commented Nov 27, 2023

hamidzr commented Nov 28, 2023

dzhu left a comment

hamidzr commented Dec 4, 2023 •

edited

Loading

hamidzr commented Apr 2, 2024 •

edited

Loading

codecov bot commented Apr 2, 2024 •

edited

Loading

erikwilson Apr 2, 2024 •

edited

Loading

erikwilson Apr 2, 2024

hamidzr Apr 16, 2024

erikwilson Apr 16, 2024

hamidzr Apr 16, 2024

hamidzr commented Apr 16, 2024

chore: upgrade protoc used in CI #7935

chore: upgrade protoc used in CI #7935

Conversation

hamidzr commented Sep 19, 2023 • edited Loading

Description

Test Plan

Commentary (optional)

Checklist

Ticket

netlify bot commented Sep 19, 2023 • edited Loading

✅ Deploy Preview for determined-ui ready!

dzhu commented Nov 27, 2023

hamidzr commented Nov 28, 2023

dzhu left a comment

Choose a reason for hiding this comment

hamidzr commented Dec 4, 2023 • edited Loading

hamidzr commented Apr 2, 2024 • edited Loading

codecov bot commented Apr 2, 2024 • edited Loading

Codecov Report

erikwilson Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

erikwilson Apr 2, 2024

Choose a reason for hiding this comment

hamidzr Apr 16, 2024

Choose a reason for hiding this comment

erikwilson Apr 16, 2024

Choose a reason for hiding this comment

hamidzr Apr 16, 2024

Choose a reason for hiding this comment

hamidzr commented Apr 16, 2024

hamidzr commented Sep 19, 2023 •

edited

Loading

netlify bot commented Sep 19, 2023 •

edited

Loading

hamidzr commented Dec 4, 2023 •

edited

Loading

hamidzr commented Apr 2, 2024 •

edited

Loading

codecov bot commented Apr 2, 2024 •

edited

Loading

erikwilson Apr 2, 2024 •

edited

Loading