-
Notifications
You must be signed in to change notification settings - Fork 7.2k
refactor(ci): Unique env name for images #60596
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
base: master
Are you sure you want to change the base?
Conversation
Blocker for spec discovery. wanda files raised in the error message all share the same name attribute of name: "$IMAGE_TO", so spec discovery cannot successfully tie a single name -> Wanda Spec. This property is required, since the discovery uses the name to resolve building pre-req images of form cr.ray.io/rayproject/{name}
Topic: refac-names
Signed-off-by: andrew <andrew@anyscale.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the CI configuration to use unique environment variable names for Docker image identifiers across different build workflows (core, llm, ml, rllib). Previously, a generic $IMAGE_TO variable was used in multiple wanda.yaml files, causing conflicts during the spec discovery phase. By introducing prefixed variables like $CORE_IMAGE_TO, $LLM_IMAGE_TO, etc., each Wanda spec now has a unique name, resolving the issue. The changes are applied consistently across all relevant Buildkite pipeline configurations and Wanda spec files. The refactoring is clear, correct, and directly addresses the problem described.
| env: | ||
| IMAGE_FROM: cr.ray.io/rayproject/oss-ci-base_build-py{{matrix}} | ||
| IMAGE_TO: corebuild-py{{matrix}} | ||
| CORE_IMAGE_FROM: cr.ray.io/rayproject/oss-ci-base_build-py{{matrix}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not really like this restriction that env vars have to be unique repo wise now.. these env vars should be locally.
we should have a way to separate wanda's input and global variable?
or why do we need this rename? this feels that some abstraction is not doing right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only the name variable that has to be globally unique, and it's for spec discovery. This is an issue for these specific cases that define name as the same environment variable IMAGE_FROM
I'm open to ideas, but as-is, we need to be able to find a way to resolve what qyx.wanda.yaml would depend on in the case that we've exported MYENV=foo:
foo.wanda.yaml
name: "foo"
foo-other.wanda.yaml
name: "$MYENV"
qux.wanda.yaml
name: "qux"
froms: cr.ray.io/rayproject/foo
Exercise:
$ MYENV=foo wanda qux.wanda.yaml
# Does this resolve to depending on foo, or foo-other?
I'll need ot reflect more on how to not overcomplicate this, but this could maybe be where we depend on some form of params that restricts the Wanda file's name schema
Blocker for spec discovery. wanda files raised in the error message all share the same name attribute of name: "$IMAGE_TO", so spec discovery cannot successfully tie a single name -> Wanda Spec. This property is required, since the discovery uses the name to resolve building pre-req images of form cr.ray.io/rayproject/{name}
Topic: refac-names
Signed-off-by: andrew andrew@anyscale.com