Skip to content
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

Fix kind load command for kfp image. #882

Closed
wants to merge 1 commit into from
Closed

Conversation

revit13
Copy link
Collaborator

@revit13 revit13 commented Dec 15, 2024

Why are these changes needed?

In recent transform refactoring PRs, such as those for ededup and tokenization, the kind load command is executed twice for the transform image in the kfp tests. This happens instead of executing it once for the kfp-data-processing image and once for the transform image. This issue can be observed in the logs of KFP runs in these PRs by searching for the kind load string.

This PR aims to address this issue.

Explanation of the Error:

To understand why kfp-data-processing:latest quay.io/dataprep1/data-prep-kit/kfp-data-processing:latest is not loaded as expected in kfp-load-kind target let's look at the Makefile in kfp/kfp_ray_components:

in its first line it includes .make.defaults which defines DOCKER_REMOTE_IMAGE:

DOCKER_REMOTE_IMAGE=$(DOCKER_REGISTRY_ENDPOINT)/$(DOCKER_IMAGE_NAME):$(DOCKER_IMAGE_VERSION)

This is a recursively expanded variable, meaning its value is dynamically recalculated each time it is referenced, using the current values of any variables it depends on.

The recent transform refactoring PRs define the DOCKER_IMAGE_NAME variable in kfp_ray/Makefile as follows:

DOCKER_IMAGE_NAME = $(TRANSFORM_NAME)-$(TRANSFORM_RUNTIME)

Which is also recursively expanded variable, causing DOCKER_REMOTE_IMAGE to be as follows:

DOCKER_REMOTE_IMAGE=$(DOCKER_REGISTRY_ENDPOINT)/$(TRANSFORM_NAME)-$(TRANSFORM_RUNTIME):$(DOCKER_IMAGE_VERSION)

This explains why when executing kind-load-image target DOCKER_REMOTE_IMAGE equals to quay.io/dataprep1/data-prep-kit/ededup-ray:latest.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Signed-off-by: Revital Sur <[email protected]>
@revit13 revit13 requested review from roytman and touma-I December 15, 2024 10:45
@revit13 revit13 marked this pull request as draft December 15, 2024 11:49
@revit13 revit13 marked this pull request as ready for review December 15, 2024 20:16
@revit13
Copy link
Collaborator Author

revit13 commented Dec 18, 2024

Addressed in #890

@revit13 revit13 closed this Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant