Skip to content
This repository has been archived by the owner on Jul 17, 2024. It is now read-only.

Test failed due to drop local deps #147

Closed
zwpaper opened this issue Aug 11, 2023 · 19 comments
Closed

Test failed due to drop local deps #147

zwpaper opened this issue Aug 11, 2023 · 19 comments

Comments

@zwpaper
Copy link
Member

zwpaper commented Aug 11, 2023

The test will fail because the agent is using the old version.

cc @zwpaper

Originally posted by @kemingy in #146 (comment)

#138 (comment)

@kemingy
Copy link
Member

kemingy commented Aug 11, 2023

Found an article about golang monorepo: earthly monorepo.

BTW this might also affect the helm chart for release: https://github.com/tensorchord/openmodelz-charts

cc @gaocegege

@zwpaper
Copy link
Member Author

zwpaper commented Aug 11, 2023

#138 (comment)

The context here is that you won't need to build the release by yourself locally. It's already done in the CI.

people may need to reproduce the release build, so that people, at least I used to build the release locally, sometimes with modifying the released code.

It's not a local dependencies, it's dev with local changes. We won't release the package with local dependencies.

tag should mean a release? I found the source code in tags is also with local dependencies

replace github.com/tensorchord/openmodelz/agent => ../agent

This PR makes it easy to use go install (recommend using pip since some go versions will break the code) but also harms local development. Building mdz with local agent code is necessary. Otherwise, you won't be able to add new features.

first of all, go install would be a better choice for a go developer like me, I can test the newest source code or switch to a specific version I want, I know it only builds the source code and put a binary under $GOPATH/bin.

for harmed local development, I propose an ENV like MDZ_DEPS=local, we can export the env wherever need to have local dependencies and build it with local dependencies automatically.

another point, may I ask why separate those packages into standalone ones if there are so closely related?

a local dependency breaks it from the go community, people are hard to integrate mdz into some other golang packages

@gaocegege
Copy link
Member

another point, may I ask why separate those packages into standalone ones if there are so closely related?

They are different components. OpenModelZ is a typical mono repo.

@gaocegege
Copy link
Member

As for the release, we will create the same version for all these components. Seems that local dependencies are the most convenient way.

WDYT @zwpaper

@kemingy
Copy link
Member

kemingy commented Aug 11, 2023

  • I don't mean that you cannot run make build-release, I mean local-dev should be prioritised since it's used frequently for developers.
  • It's the same thing. When you release it with the replace, you're still using the tagged version (it's only triggered by new tags), instead of some latest commits.
  • go install is a good-to-have feature, we will use pip as the main installation since it's more popular for the target user.
  • I think the most frequently used feature should be the default choice.
  • People won't need to integrate with mdz since it's just a CLI, they will integrate with agent or others.

@zwpaper
Copy link
Member Author

zwpaper commented Aug 11, 2023

created a PR to propose an update for previous changes,

#148

@kemingy
Copy link
Member

kemingy commented Aug 11, 2023

Another thing: I have to git checkout mdz/go.mod mdz/go.sum every time before committing the changes.

@zwpaper
Copy link
Member Author

zwpaper commented Aug 11, 2023

Hmmm, what is would like to say is that in-tree local dependencies are really a bad choice for Go development, even the article https://earthly.dev/blog/golang-monorepo/#importing-local-go-modules-in-a-monorepo show an example for monorepo in Go, but it looks like earthly is not actually have any in-tree local dependencies except the example.

but anyway, you guys are the most frequent developers, should be considered first...

@zwpaper
Copy link
Member Author

zwpaper commented Aug 11, 2023

let me submit a PR to revert the changes

@kemingy
Copy link
Member

kemingy commented Aug 11, 2023

Is there any downside of the monorepo with local dependencies?

@zwpaper
Copy link
Member Author

zwpaper commented Aug 11, 2023

it is mostly about go community, with the local dependencies,

  1. we can only use omz by cloning the repo and building it, can not integrate, I know mdz is a cli tool, but agent also has local dependencies, and cli also could be a dependency like https://github.com/containerd/nerdctl/blob/b7c7c4988f0cef02c97ad4991d1be015797a7215/go.mod#L28
  2. lost the version controlling, but it should be decided when you choice monorepo.

I am also curious about why not treat openmodelz as a go package, like

- openmodelz
- go.mod
- go.sum
  - cmd
    - mdz
- agent
- ingress-operator

it seems to be more of a Go monorepo, people could integrate github.com/tc/openmodelz/agent, github.com/tc/openmodelz/agent as they like, and openmodelz could release in a monorepo way.

@zwpaper
Copy link
Member Author

zwpaper commented Aug 11, 2023

the most famous local dependencies user I know is k8s https://github.com/kubernetes/kubernetes/blob/master/go.mod#L250-L280, but they also created and maintained a standalone repository for each dependency, which is not easy and not necessary IMHO.

even k8s has a root go.mod to make k/k a monorepo go package.

@zwpaper
Copy link
Member Author

zwpaper commented Aug 11, 2023

but like I said, I don't mean harm...

a revert PR was created #149, feel free to revert the changes, it totally ok to me.

@gaocegege
Copy link
Member

We keep them separate because users may reply on the agent directly. agent is a library, instead of a exe binary. Just FYI

@zwpaper
Copy link
Member Author

zwpaper commented Aug 11, 2023

We keep them separate because users may reply on the agent directly. agent is a library, instead of a exe binary. Just FYI

yes, but with those 2 lines, users can not use agent as a library...

github.com/tensorchord/openmodelz/ingress-operator => ../ingress-operator
github.com/tensorchord/openmodelz/modelzetes => ../modelzetes

like I said, if openmodelz is release as a single go package with only one go.mod under the root dir, users can also use agent as a dependency easily, and not break any of the go community usage.

or maybe I get something wrong? as I was really new to openmodelz

@gaocegege
Copy link
Member

like I said, if openmodelz is release as a single go package with only one go.mod under the root dir, users can also use agent as a dependency easily, and not break any of the go community usage.

Yeah, I think so.

@gaocegege
Copy link
Member

after the discussion and getting familiar with openmodelz, I was really curious about why prefer separate openmodelz into 4 go packages rather than a single one, if the 4 packages are so closely related...

Actually we do not prefer it. We just start every component as a standalone project, then iterate based on them.

I think we could maintain one go.mod.

@zwpaper
Copy link
Member Author

zwpaper commented Aug 11, 2023

it's only some personal opinions from me, without some more deep investigation or real practice within openmodelz.

it's more important with the opinions from @kemingy, you guys can discuss the idea offline as it may be more effective than posting on GitHub.

@kemingy
Copy link
Member

kemingy commented Aug 11, 2023

I think we'd better migrate to one go.mod file.

@kemingy kemingy closed this as completed Aug 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants