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

[KFP] Obtain the Ray cluster run ID from the user for KFP v2. #956

Merged
merged 18 commits into from
Jan 27, 2025

Conversation

revit13
Copy link
Collaborator

@revit13 revit13 commented Jan 20, 2025

Why are these changes needed?

In KFPv2 dsl.RUN_ID_PLACEHOLDER is deprecated and cannot be used since SDK 2.5.0. On another hand we cannot create
a unique string in a component (at runtime) and pass it to the clean_up_task of ExitHandler, due to
kubeflow/pipelines#10187.
Therefore, meantime the user is requested to insert a unique Ray cluster name at Run creation time.

Issue

#977

@revit13 revit13 marked this pull request as draft January 20, 2025 12:03
Signed-off-by: Revital Sur <[email protected]>
Signed-off-by: Revital Sur <[email protected]>
Signed-off-by: Revital Sur <[email protected]>
Signed-off-by: Revital Sur <[email protected]>
Signed-off-by: Revital Sur <[email protected]>
@revit13 revit13 marked this pull request as ready for review January 22, 2025 11:17
@revit13 revit13 requested a review from roytman January 22, 2025 11:18
@roytman
Copy link
Member

roytman commented Jan 22, 2025

Please update the PR explanation:
"meantime the user is requested to insert a unique string created at compilation time." -> "meantime the user is requested to insert a unique Ray cluster name at Run creation time."

# In KFPv2 dsl.RUN_ID_PLACEHOLDER is deprecated and cannot be used since SDK 2.5.0. On another hand we cannot create
# a unique string in a component (at runtime) and pass it to the `clean_up_task` of `ExitHandler`, due to
# https://github.com/kubeflow/pipelines/issues/10187. Therefore, meantime the user is requested to insert
# a unique string created at compilation time.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at run creation time

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks

func=compute_exec_params_func, base_image=base_kfp_image
)
else:
compute_exec_params_op = comp.create_component_from_func(func=compute_exec_params_func, base_image=base_kfp_image)
# create Ray cluster
create_ray_op = comp.load_component_from_file("../../../kfp_ray_components/createRayComponent.yaml")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be component_spec_path + "createRayClusterComponent.yaml"
and below

@@ -57,11 +57,16 @@ Ray cluster. For each step we have to define a component that will execute them:
```python
# components
base_kfp_image = "quay.io/dataprep1/data-prep-kit/kfp-data-processing:0.0.2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it the correct image name?

@@ -107,6 +112,7 @@ The input parameters section defines all the parameters required for the pipelin
The parameters used here are as follows:

* ray_name: name of the Ray cluster
* ray_run_id_KFPv2: Ray cluster unique ID used only in KFP v2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not in the pipeline parameters; it should be around line 91

@@ -111,9 +99,11 @@ def {{ pipeline_name }}(
ray_name: str = "{{ pipeline_name }}-kfp-ray", # name of Ray cluster
# Add image_pull_secret and image_pull_policy to ray workers if needed
{%- if image_pull_secret != "" %}
ray_run_id_KFPv2: str = "",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to define it twice? Here and at L 106?

@@ -139,6 +128,7 @@ def code2parquet(
"""
Pipeline to execute NOOP transform
:param ray_name: name of the Ray cluster
:param ray_run_id_KFPv2: string holding the id used for the Ray cluster used only in KFP v2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a unique string id, ..."

Signed-off-by: Revital Sur <[email protected]>
Signed-off-by: Revital Sur <[email protected]>
Signed-off-by: Revital Sur <[email protected]>
Signed-off-by: Revital Sur <[email protected]>
Signed-off-by: Revital Sur <[email protected]>
Signed-off-by: Revital Sur <[email protected]>
Signed-off-by: Revital Sur <[email protected]>
@@ -113,6 +101,7 @@ def compute_exec_params_func(
)
def code2parquet(
ray_name: str = "code2parquet-kfp-ray", # name of Ray cluster
ray_run_id_KFPv2: str = "", # Ray cluster unique ID used only in KFP v2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we share it with both versions if we don't need it for KFPv1?

@@ -21,6 +21,7 @@ Another useful feature of the KFP v2 is the `Json` editor for the `dict` type in
- It creates just one run that includes all the nested transfroms and their sub-tasks.
- No need for additional component as `executeSubWorkflowComponent.yaml`. All the implementation in the same pipeline file.
- In superpipelines of KFP v1 there exists an option to override the common parameters with specific values for each one of the transforms. This option is missing in the KFP v2 superpipelines.
- In kfp V2 pipelines the user is requested to insert a unique string for the ray cluster created at run creation time (called `ray_run_id_KFPv2`). This is because in KFPv2 dsl.RUN_ID_PLACEHOLDER is deprecated and cannot be used since SDK 2.5.0.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"and we cannot generate a unique string at run-time, see kubeflow/pipelines#10187"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks

@@ -62,6 +62,7 @@ def super_pipeline(
p2_skip: bool = False,
p2_noop_sleep_sec: int = 10,
p2_ray_name: str = "noop-kfp-ray",
p2_ray_run_id_KFPv2: str = "",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can it be p1_ parameter, so users don't have to add it for each step?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks

Signed-off-by: Revital Sur <[email protected]>
Copy link
Member

@roytman roytman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@touma-I touma-I merged commit 8e03629 into IBM:dev Jan 27, 2025
126 checks passed
@roytman roytman mentioned this pull request Jan 28, 2025
2 tasks
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.

3 participants