Reproducible builds #6

tarilabs · 2023-09-20T10:16:49Z

use makefile to automate deps install
the .gitignore did not account for file relocations from 97d4c5f

deleted: pkg/ml_metadata/proto/metadata_store.pb.go
deleted: pkg/ml_metadata/proto/metadata_store_service.pb.go
deleted: pkg/ml_metadata/proto/metadata_store_service_grpc.pb.go

bump Go version:

 # github.com/sashamelentyev/usestdlibvars/pkg/analyzer/internal/mapping
Error: ../../../go/pkg/mod/github.com/sashamelentyev/usestdlibvars@v1.24.0/pkg/analyzer/internal/mapping/mapping.go:164:7: undefined: time.DateTime
Error: ../../../go/pkg/mod/github.com/sashamelentyev/usestdlibvars@v1.24.0/pkg/analyzer/internal/mapping/mapping.go:165:7: undefined: time.DateOnly
Error: ../../../go/pkg/mod/github.com/sashamelentyev/usestdlibvars@v1.24.0/pkg/analyzer/internal/mapping/mapping.go:166:7: undefined: time.TimeOnly
note: module requires Go 1.20

GHA for protoc installation per requirements

Description

setup a GHA action for dry run and reproducible builds according to requirements listed in README (or to be amended)
remove files which were supposed not to be checked-in according to .gitignore
accounts also for: [pull] main from kubeflow:main model-registry#4 (comment)

How Has This Been Tested?

see https://github.com/tarilabs/model-registry/actions/runs/6247075284/job/16958971408

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work: see https://github.com/tarilabs/model-registry/actions/runs/6247075284/job/16958971408

lampajr

Thanks a lot @tarilabs for the idea of automating the build and making it reproducible!

Added just some comments/doubt below

.gitignore

lampajr · 2023-09-20T11:55:17Z

README.md

@@ -2,7 +2,7 @@
 A go based server that implements a gRPC interface for [ml_metadata](https://github.com/google/ml-metadata/) library.
 It adds other features on top of the functionality offered by the gRPC interface.
 ## Pre-requisites:
- go >= 1.19
+- go >= 1.20


If we change this, shouldn't we change the go version in go.mod as well?

Apart from this, is it fine to have precondition on go >= 1.20 or for some reason should we keep 1.19?

I could not reproduce with Go 1.19, so I would say this must bump or else the code which is already checked in is actually broken

yeah I think the issue was during this installation: go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest (or maybe not just this one) so if for some reason we need to keep 1.19 we might think to set older dependency version instead of latest.

Another question is: is it safe to use the latest version for those deps? I do not know how fast Go world is going so we might be in the same situation sooner if they keep releasing new versions (not backward compatible) frequently. Maybe this is not the right moment for this discussion :)

For the "latest" I believe we can keep as-is for the time being, and to get started with. We can always improve on that later (and separate PR)

If the code doesn't really work with 1.19, I'd prefer to have a version bump change in another commit so we can refer to it as a separate bugfix.

If the code doesn't really work with 1.19, I'd prefer to have a version bump change in another commit so we can refer to it as a separate bugfix.

it was, but code-policy requires squashed commit, and reproducible build is the subject here (including, in this case, the version bump)

Let's go with changes from @lampajr . As far as we can, we should try and support the last 3 go releases, which is 1.19, since 1.21 is the latest right now.

isinyaaa · 2023-09-20T12:39:55Z

Makefile

+deps:
+	go install github.com/99designs/gqlgen@latest
+	go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest
+	go install google.golang.org/protobuf/cmd/protoc-gen-go@latest
+	go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@latest


We should probably add the go-enum dependency here as well as per Andrea's note on #3.

I agree, but depending on which PR gets in first, we will rebase/modify accordingly.

isinyaaa · 2023-09-20T12:40:30Z

Makefile

+.PHONY: deps
+deps:
+	go install github.com/99designs/gqlgen@latest
+	go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest


Also, this conflicts with the new instructions from #4.

I agree, but depending on which PR gets in first, we will rebase/modify accordingly.

isinyaaa

Can you separate the version bump (as commit [1/4]), from the gitignore changes (as commit [2/4]), add the new Makefile rules (as commit [3/4]), and then finally add the CI? This way we can keep nice atomic changes.

Also, as mentioned in my comment on #3 we should stick to git's commit conventions.

For some reason I couldn't find a CI run on your branch, can you provide a link to a successful run with the suggested build.yml?

Either way, nice work here, let's keep project maintenance in check :)

tarilabs · 2023-09-20T12:53:39Z

Can you separate the version bump (as commit [1/4]), from the gitignore changes (as commit [2/4]), add the new Makefile rules (as commit [3/4], and then finally add the CI? This way we can keep nice atomic changes.

no i cannot. as explained code policy wants PR with squashed commits 🤷
it was the case before.

Also, as mentioned in my comment on #3 we should stick to git's commit conventions.

see above, the squashed commit text respects the result of that once squashed

For some reason I couldn't find a CI run on your branch, can you provide a link to a successful run with the suggested build.yml?

I linked it in the original description:

How Has This Been Tested?
see https://github.com/tarilabs/model-registry/actions/runs/6247075284/job/16958971408

isinyaaa · 2023-09-20T12:57:22Z

Can you separate the version bump (as commit [1/4]), from the gitignore changes (as commit [2/4]), add the new Makefile rules (as commit [3/4], and then finally add the CI? This way we can keep nice atomic changes.

no i cannot. as explained code policy wants PR with squashed commits 🤷 it was the case before.

Hmm, about the code policy, does that make sense? I wouldn't say so, as it's a lot easier to keep atomic changes. Either way, I interpret it differently: "the commits are squashed" as in you aren't making a change then fixing it in an upcoming commit in the same series -- so we don't have to refer to multiple changes for a single atomic modification.

EDIT: A little thought experiment: if we were to squash all commits in every PR, then we'd either get massive changes on most PRs or we'd have to separate everything in multiple PRs, either way, this isn't helpful at all for reviews or for the project history.

Also, as mentioned in my comment on #3 we should stick to git's commit conventions.

see above, the squashed commit text respects the result of that once squashed

For some reason I couldn't find a CI run on your branch, can you provide a link to a successful run with the suggested build.yml?

I linked it in the original description:

How Has This Been Tested?
see https://github.com/tarilabs/model-registry/actions/runs/6247075284/job/16958971408

Sorry, I missed that, my bad.

tarilabs · 2023-09-20T13:36:11Z

Hmm, about the code policy, does that make sense? I wouldn't say so, as it's a lot easier to keep atomic changes (...)

I am empathic with this, but on other hand I don't make the rules 😬
I think for the time being it is what it is, and we can ask for derogation in using "squash and merge" on github.

Sorry, I missed that, my bad

No worries.
I would like to proceed swiftly and improve on process progressively.
I noted the "build is broken", this fixes all the necessary broken points and ensure non-regression with the GHA.
Hope this ☝️ is an alternative good "title" if preferred.

tarilabs · 2023-09-20T15:43:40Z

rebased to account for #4

dhirajsb

lgtm, we can add more checks like uncommitted code check in another PR.

dhirajsb · 2023-09-20T14:47:51Z

README.md

@@ -2,7 +2,7 @@
 A go based server that implements a gRPC interface for [ml_metadata](https://github.com/google/ml-metadata/) library.
 It adds other features on top of the functionality offered by the gRPC interface.
 ## Pre-requisites:
- go >= 1.19
+- go >= 1.20


Let's go with changes from @lampajr . As far as we can, we should try and support the last 3 go releases, which is 1.19, since 1.21 is the latest right now.

.gitignore

.github/workflows/build.yml

lampajr

Some suggestion, moreover I tried to run the workflow locally but it keeps failing for this:

:error::Failed to run: Error: failed to get action config: connect ENETUNREACH 2606:50c0:8002::154:443, Error: failed to get action config: connect ENETUNREACH 2606:50c0:8002::154:443%0A    at getConfig (/run/act/actions/golangci-golangci-lint-action@v3/dist/run/index.js:67135:15)%0A    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)%0A    at async findLintVersion (/run/act/actions/golangci-golangci-lint-action@v3/dist/run/index.js:67156:20)%0A    at async prepareLint (/run/act/actions/golangci-golangci-lint-action@v3/dist/run/index.js:66704:27)%0A    at async prepareEnv (/run/act/actions/golangci-golangci-lint-action@v3/dist/run/index.js:66765:22)

It seems something related to the golangci-lint action but I did not find yet the root cause.

.github/workflows/build.yml

Makefile

README.md

.github/workflows/build.yml

tarilabs · 2023-09-21T11:58:28Z

To highlight why I was suggesting not to check-in to version-control the generated files, the check fails because anyone might have a slightly different environment:

diff --git a/internal/ml_metadata/proto/metadata_store.pb.go b/internal/ml_metadata/proto/metadata_store.pb.go
index 0d89cd2..ee2d0db 100644
--- a/internal/ml_metadata/proto/metadata_store.pb.go
+++ b/internal/ml_metadata/proto/metadata_store.pb.go
@@ -16,7 +16,7 @@
 // Code generated by protoc-gen-go. DO NOT EDIT.
 // versions:
 //     protoc-gen-go v1.31.0
-//     protoc        v3.20.3
+//     protoc        v4.24.2
 // source: ml_metadata/proto/metadata_store.proto
 
 package proto
diff --git a/internal/ml_metadata/proto/metadata_store_service.pb.go b/internal/ml_metadata/proto/metadata_store_service.pb.go
index fadd1ce..64ef5ef 100644
--- a/internal/ml_metadata/proto/metadata_store_service.pb.go
+++ b/internal/ml_metadata/proto/metadata_store_service.pb.go
@@ -16,7 +16,7 @@
 // Code generated by protoc-gen-go. DO NOT EDIT.
 // versions:
 //     protoc-gen-go v1.31.0
-//     protoc        v3.20.3
+//     protoc        v4.24.2
 // source: ml_metadata/proto/metadata_store_service.proto
 
 package proto
diff --git a/internal/ml_metadata/proto/metadata_store_service_grpc.pb.go b/internal/ml_metadata/proto/metadata_store_service_grpc.pb.go
index cd3945f..7661927 100644
--- a/internal/ml_metadata/proto/metadata_store_service_grpc.pb.go
+++ b/internal/ml_metadata/proto/metadata_store_service_grpc.pb.go
@@ -16,7 +16,7 @@
 // Code generated by protoc-gen-go-grpc. DO NOT EDIT.
 // versions:
 // - protoc-gen-go-grpc v1.3.0
-// - protoc             v3.20.3
+// - protoc             v4.24.2
 // source: ml_metadata/proto/metadata_store_service.proto
 
 package proto

isinyaaa · 2023-09-21T12:09:27Z

.gitignore

@@ -3,3 +3,4 @@
 model-registry
 metadata.sqlite.db
 vendor
+.DS_Store


While I appreciate this as a Mac user, you should probably ignore that in your ~/.gitignore file (or the project's local .git/info/exclude)

Most the projects I've worked on put this in the project's .gitignore so that doesn't "expect" contributors to have additional local-setup (say someone raise a PR, is likely they might not notice this slips-in).

Example:

https://github.com/quarkusio/quarkus/blob/main/.gitignore#L1

https://github.com/apache/camel/blob/main/.gitignore#L6

With the idea is that you make the life easier for any contributors.

But seems to me this is what is preferred to the group, acting on it 👍

isinyaaa · 2023-09-21T12:13:25Z

To highlight why I was suggesting not to check-in to version-control the generated files, the check fails because anyone might have a slightly different environment:

I definitely agree with you. We should only make it easy to regen those. I still haven't tried my hand at that though, do we have any reasons to avoid it?

dhirajsb · 2023-09-21T15:52:25Z

We want to make sure the binaries generated are exactly the same using exactly the same tools.

This is especially important when eventually this becomes a managed service.
SREs hate having to setup a development environment/CI doing it like in the git actions, and they don't like environment specific side effects during builds.

So we should have a bin directory in the project where we install tools in the Makefile. We can use targets like:

bin/golangci-lint:
    curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b ./bin v1.54.2

And make these tools dependencies of other targets like gen/* for example.
For the go install commands, we can set the GOBIN directory like so:

GOBIN=`pwd`/bin go install github.com/searKing/golang/tools/go-enum@v1.2.97

And lock down the tool versions to ensure repeatability.

@tarilabs if you wish you can do it in this PR, or I can create another PR if you are tired of working on this. 😄

dhirajsb · 2023-09-21T17:22:39Z

@lampajr @isinyaaa do you have blocking concerns about this PR? If they are things we can improve or sort out later, please remove the change request so we can get these changes in soon.

isinyaaa · 2023-09-21T17:40:49Z

I'll approve once it's properly rebased and commits are looking good. Even if we're squashing everything the description wouldn't make any sense as is rn.

1. use makefile to automate deps install 2. setup basic GHA to ensure makefile build is reproducible

tarilabs · 2023-09-21T21:56:14Z

PR reworked per f2d9469 description and externalized "uncommitted changes check" to be dealt later in opendatahub-io/model-registry#16

tarilabs · 2023-09-22T15:31:53Z

on no-veto(s) remaining, and neutral or approved reviews, proceeding to merge.

Merge model registry codebase from https://github.com/opendatahub-io/model-registry

tarilabs requested review from dhirajsb, lampajr and isinyaaa September 20, 2023 10:16

lampajr reviewed Sep 20, 2023

View reviewed changes

isinyaaa reviewed Sep 20, 2023

View reviewed changes

isinyaaa requested changes Sep 20, 2023

View reviewed changes

tarilabs force-pushed the tarilabs-20230920-dryrunWithCI branch from f5d6f6a to 42cc1b4 Compare September 20, 2023 15:42

tarilabs force-pushed the tarilabs-20230920-dryrunWithCI branch from 42cc1b4 to 38ae620 Compare September 21, 2023 07:01

dhirajsb previously approved these changes Sep 21, 2023

View reviewed changes

lampajr requested changes Sep 21, 2023

View reviewed changes

.github/workflows/build.yml Show resolved Hide resolved

.github/workflows/build.yml Outdated Show resolved Hide resolved

tarilabs dismissed dhirajsb’s stale review via 267daa1 September 21, 2023 08:42

tarilabs force-pushed the tarilabs-20230920-dryrunWithCI branch from 4e642f1 to 771483c Compare September 21, 2023 08:44

lampajr requested changes Sep 21, 2023

View reviewed changes

Makefile Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

.github/workflows/build.yml Outdated Show resolved Hide resolved

isinyaaa reviewed Sep 21, 2023

View reviewed changes

tarilabs mentioned this pull request Sep 21, 2023

GHA enhancement no uncommitted changes #16

Closed

tarilabs force-pushed the tarilabs-20230920-dryrunWithCI branch from 83b2f5c to afb8e8a Compare September 21, 2023 21:45

Reproducible builds

f2d9469

1. use makefile to automate deps install 2. setup basic GHA to ensure makefile build is reproducible

tarilabs force-pushed the tarilabs-20230920-dryrunWithCI branch from afb8e8a to f2d9469 Compare September 21, 2023 21:49

lampajr approved these changes Sep 22, 2023

View reviewed changes

isinyaaa approved these changes Sep 22, 2023

View reviewed changes

tarilabs merged commit 5f25946 into opendatahub-io:main Sep 22, 2023

tarilabs mentioned this pull request Sep 25, 2023

Establish GitHub build #24

Closed

isinyaaa pushed a commit to isinyaaa/model-registry-bf4-kf that referenced this pull request Feb 21, 2024

Merge pull request opendatahub-io#6 from dhirajsb/merge-odh-mr

198fb0e

Merge model registry codebase from https://github.com/opendatahub-io/model-registry

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reproducible builds #6

Reproducible builds #6

tarilabs commented Sep 20, 2023

lampajr left a comment

lampajr Sep 20, 2023 •

edited

Loading

tarilabs Sep 20, 2023

lampajr Sep 20, 2023

tarilabs Sep 20, 2023

isinyaaa Sep 20, 2023

tarilabs Sep 20, 2023

dhirajsb Sep 20, 2023

isinyaaa Sep 20, 2023

tarilabs Sep 20, 2023

isinyaaa Sep 20, 2023 •

edited

Loading

tarilabs Sep 20, 2023

isinyaaa left a comment •

edited

Loading

tarilabs commented Sep 20, 2023

isinyaaa commented Sep 20, 2023 •

edited

Loading

tarilabs commented Sep 20, 2023

tarilabs commented Sep 20, 2023

dhirajsb left a comment

dhirajsb Sep 20, 2023

lampajr left a comment

tarilabs commented Sep 21, 2023

isinyaaa Sep 21, 2023

tarilabs Sep 21, 2023

isinyaaa commented Sep 21, 2023

dhirajsb commented Sep 21, 2023

dhirajsb commented Sep 21, 2023

isinyaaa commented Sep 21, 2023

tarilabs commented Sep 21, 2023

tarilabs commented Sep 22, 2023

Reproducible builds #6

Reproducible builds #6

Conversation

tarilabs commented Sep 20, 2023

Description

How Has This Been Tested?

Merge criteria:

lampajr left a comment

Choose a reason for hiding this comment

lampajr Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isinyaaa Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isinyaaa left a comment • edited Loading

Choose a reason for hiding this comment

tarilabs commented Sep 20, 2023

isinyaaa commented Sep 20, 2023 • edited Loading

tarilabs commented Sep 20, 2023

tarilabs commented Sep 20, 2023

dhirajsb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lampajr left a comment

Choose a reason for hiding this comment

tarilabs commented Sep 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isinyaaa commented Sep 21, 2023

dhirajsb commented Sep 21, 2023

dhirajsb commented Sep 21, 2023

isinyaaa commented Sep 21, 2023

tarilabs commented Sep 21, 2023

tarilabs commented Sep 22, 2023

lampajr Sep 20, 2023 •

edited

Loading

isinyaaa Sep 20, 2023 •

edited

Loading

isinyaaa left a comment •

edited

Loading

isinyaaa commented Sep 20, 2023 •

edited

Loading