feat(sdk): add runtime resource requests. Fixes #1956 #5447

NikeNano · 2021-04-08T18:30:01Z

Signed-off-by: NikeNano niklas.sven.hansson@gmail.com

Description of your changes:

UPDATE: linking don't seem to work, relates to: #1956

Checklist:

The title for your pull request (PR) should follow our title convention. Learn more about the pull request title convention used in this repository.

Do you want this pull request (PR) cherry-picked into the current release branch?

Learn more about cherry-picking updates into the release branch.

NikeNano · 2021-04-08T20:53:54Z

/assign @hongye-sun

NikeNano · 2021-04-13T18:49:17Z

Maybe you could take a look on this @Ark-kun? Thanks.

NikeNano · 2021-04-22T18:54:21Z

@numerology could you please take a look? Thanks

NikeNano · 2021-05-03T10:55:30Z

@Bobgy do you have the possibility to look at this? Have been hard to get a review on this one :(

NikeNano · 2021-05-12T04:37:26Z

@capri-xiyue do you have the opportunity to look at this?

capri-xiyue · 2021-05-12T19:19:40Z

@capri-xiyue do you have the opportunity to look at this?

This PR looks good to me. But I don't have a lot of experience on the sdk side.
@chensun @Ark-kun Can you help review this PR?

capri-xiyue · 2021-05-12T19:20:54Z

/retest

NikeNano · 2021-05-12T19:21:08Z

@capri-xiyue do you have the opportunity to look at this?
This PR looks good to me. But I don't have a lot of experience on the sdk side.
@chensun @Ark-kun Can you help review this PR?

Thanks @capri-xiyue :)

samples/core/runtime_resource_requests/resource_request.py

sdk/python/kfp/compiler/_op_to_template.py

sdk/python/kfp/dsl/_container_op.py

samples/core/runtime_resource_requests/resource_request.py

Ark-kun · 2021-05-13T02:49:19Z

Thank you for this PR. I've left couple of comments.

NikeNano · 2021-05-31T06:17:37Z

Pushed some changes, will double check the tests as well before it is ready for a new review.

NikeNano · 2021-06-03T09:23:32Z

@chensun and @Ark-kun could could you please take a look again?

tiru1930 · 2021-06-03T10:18:06Z

@NikeNano is this merged to master , which version sdk i can use to get benefit of this approach

NikeNano · 2021-06-03T10:25:56Z

@NikeNano is this merged to master , which version sdk i can use to get benefit of this approach

Not yet @tiru1930 but hopefully soon :)

chensun

Thanks @NikeNano. Only a couple nitpicks otherwise LGTM.
The test case being added looks awesome.

chensun · 2021-06-04T08:53:06Z

samples/core/resource_spec/runtime_resource_request.py

+        .set_cpu_limit(resouce_task.outputs['cpu'])\
+        .set_cpu_request('200m')
+
+        # Disable cache for KFP v1 mode.


nit: indentation for the comment is off.

chensun · 2021-06-04T08:55:40Z

sdk/python/kfp/compiler/_op_to_template.py

@@ -288,6 +288,24 @@ def _op_to_template(op: BaseOp):
        template['volumes'] = [convert_k8s_obj_to_json(volume) for volume in processed_op.volumes]
        template['volumes'].sort(key=lambda x: x['name'])

+    # Runtime resource requests
+    if isinstance(op, dsl.ContainerOp) and ("resources" in op.container.keys()):


nit: use consistent quotes.

chensun · 2021-06-04T08:58:01Z

/retest

chensun · 2021-06-04T09:18:59Z

@NikeNano: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
kubeflow-pipelines-samples-v2 2888390 link /test kubeflow-pipelines-samples-v2

The test added in this PR failed. The error was:

invalid spec: templates.runtime-resource-request-pipeline.tasks.training-op templates.training-op: failed to resolve {{inputs.parameters.generate-resouce-request-memory}}

NikeNano · 2021-06-04T10:34:55Z

invalid spec: templates.runtime-resource-request-pipeline.tasks.training-op templates.training-op: failed to resolve {{inputs.parameters.generate-resouce-request-memory}}

Where in the logs do you find this @chensun? But it make sens, i have not updated for V2, will do so as well.

chensun · 2021-06-06T06:16:39Z

Where in the logs do you find this @chensun?

I'm not sure if you may have access to it. Right above the call stack at the end, there's this

Run details page URL:
https://4e18c21c9d33d20f-dot-datalab-vm-staging.googleusercontent.com/#/runs/details/973f40b0-f9ec-4c9d-9bea-49e8db4f6f2e

Open that link, you will find a failed test among a list of ParallelFor tasks.

But it make sens, i have not updated for V2, will do so as well.

Note that the test is v2 compatible mode in v1. I haven't debugged this, but I think the change you need would probably be in v1 compiler code.

NikeNano · 2021-06-07T05:00:06Z

I'm not sure if you may have access to it. Right above the call stack at the end, there's this

Will run it locally for debugging.

NikeNano · 2021-06-09T20:44:15Z

invalid spec: templates.runtime-resource-request-pipeline.tasks.training-op templates.training-op: failed to resolve {{inputs.parameters.generate-resouce-request-memory}}

After debugging this I found that the issues comes from

pipelines/sdk/python/kfp/dsl/_component_bridge.py

Line 616 in 703822c

if kfp.COMPILING_FOR_V2:

which result in that the:

dependencies: [generate-resouce-request]

,

        arguments:
          parameters:
          - {name: generate-resouce-request-cpu, value: '{{tasks.generate-resouce-request.outputs.parameters.generate-resouce-request-cpu}}'}
          - {name: generate-resouce-request-memory, value: '{{tasks.generate-resouce-request.outputs.parameters.generate-resouce-request-memory}}'}

and

    inputs:
      parameters:
      - {name: generate-resouce-request-cpu}
      - {name: generate-resouce-request-memory}

are removed from the generate pipeline compare to not setting mode=kfp.dsl.PipelineExecutionMode.V2_COMPATIBLE which gives the error mentioned above.

I believe this is a design decision so before I run off and change something could you elaborate on what we are trying to accomplish and why we # Override command and arguments if compiling to v2. @chensun, thank you?

chensun · 2021-06-10T09:53:19Z

I believe this is a design decision so before I run off and change something could you elaborate on what we are trying to accomplish and why we # Override command and arguments if compiling to v2. @chensun, thank you?

In v2, we use a different set of placeholders in command and arguments. For instance:

pipelines/sdk/python/kfp/v2/compiler_cli_tests/test_data/two_step_pipeline.json

Lines 51 to 56 in cc85ca5

    
           "command": [ 
        
             "sh", 
        
             "-c", 
        
             "set -e -x\necho \"$0\" | gsutil cp - \"$1\"\n", 
        
             "{{$.inputs.parameters['text']}}", 
        
             "{{$.outputs.artifacts['output_gcs_path'].uri}}"

I think the issue is this line:

pipelines/sdk/python/kfp/dsl/_component_bridge.py

Lines 620 to 621 in 703822c

    
           # limit this to v2 compiling only to avoid possible behavior change in v1. 
        
           task.inputs = list(input_params_set)

I'll think about how to fix this properly.
For now, to unblock your change, how about you create an issue assign to me, comment out the V2_COMPATIBLE test case in your PR with a reference to the issue. Then let's merge this PR to complete the feature in V1 mode.

chensun · 2021-06-10T22:08:07Z

/lgtm
/approve

google-oss-robot · 2021-06-10T22:08:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chensun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~samples/OWNERS~~ [chensun]
~~sdk/OWNERS~~ [chensun]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…ow#5447) * added resource request at runtime * fixed things * Update to use read only parameter insteadt * added test case and better example * Updated again * add the validation * add to the test suit * work in progress * update after feedback * fix the test * clean up * clean up * fix the path * add the test again * clean up * fix tests * feedback fix * comment out and clean up

google-cla bot added the cla: yes label Apr 8, 2021

google-oss-robot requested review from Ark-kun and numerology April 8, 2021 18:30

google-oss-robot added the size/M label Apr 8, 2021

NikeNano marked this pull request as draft April 8, 2021 18:33

google-oss-robot added the do-not-merge/work-in-progress label Apr 8, 2021

NikeNano marked this pull request as ready for review April 8, 2021 18:50

google-oss-robot removed the do-not-merge/work-in-progress label Apr 8, 2021

NikeNano changed the title ~~feat(sdk): add runtime resource requests. Fixes #1956~~ [WIP] feat(sdk): add runtime resource requests. Fixes #1956 Apr 8, 2021

google-oss-robot added the do-not-merge/work-in-progress label Apr 8, 2021

NikeNano changed the title ~~[WIP] feat(sdk): add runtime resource requests. Fixes #1956~~ feat(sdk): add runtime resource requests. Fixes #1956 Apr 8, 2021

google-oss-robot removed the do-not-merge/work-in-progress label Apr 8, 2021

google-oss-robot assigned hongye-sun Apr 8, 2021

capri-xiyue requested a review from chensun May 12, 2021 19:21

Ark-kun reviewed May 13, 2021

View reviewed changes

samples/core/runtime_resource_requests/resource_request.py Outdated Show resolved Hide resolved

Ark-kun reviewed May 13, 2021

View reviewed changes

sdk/python/kfp/compiler/_op_to_template.py Outdated Show resolved Hide resolved

Ark-kun reviewed May 13, 2021

View reviewed changes

sdk/python/kfp/dsl/_container_op.py Outdated Show resolved Hide resolved

Ark-kun reviewed May 13, 2021

View reviewed changes

samples/core/runtime_resource_requests/resource_request.py Outdated Show resolved Hide resolved

Bobgy assigned Ark-kun and chensun and unassigned hongye-sun May 13, 2021

update after feedback

9293431

NikeNano added 5 commits May 31, 2021 12:21

fix the test

312e850

clean up

5951916

clean up

4601608

fix the path

0f6c148

add the test again

af22f4b

NikeNano added 2 commits June 3, 2021 18:36

clean up

03bc7fc

fix tests

2888390

chensun reviewed Jun 4, 2021

View reviewed changes

feedback fix

c7eb4c2

NikeNano mentioned this pull request Jun 10, 2021

[sdk] V2_COMPATIBLE mode don't work with parameter input for setting resources. #5835

Closed

comment out and clean up

f445edf

google-oss-robot added the lgtm label Jun 10, 2021

google-oss-robot added the approved label Jun 10, 2021

google-oss-robot merged commit 5db8431 into kubeflow:master Jun 10, 2021

zrlaida mentioned this pull request Jun 30, 2021

Add in support for podSpecPatch #4877

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sdk): add runtime resource requests. Fixes #1956 #5447

feat(sdk): add runtime resource requests. Fixes #1956 #5447

NikeNano commented Apr 8, 2021 •

edited

Loading

NikeNano commented Apr 8, 2021

NikeNano commented Apr 13, 2021

NikeNano commented Apr 22, 2021

NikeNano commented May 3, 2021

NikeNano commented May 12, 2021

capri-xiyue commented May 12, 2021 •

edited

Loading

capri-xiyue commented May 12, 2021

NikeNano commented May 12, 2021

Ark-kun commented May 13, 2021

NikeNano commented May 31, 2021

NikeNano commented Jun 3, 2021

tiru1930 commented Jun 3, 2021

NikeNano commented Jun 3, 2021

chensun left a comment

chensun Jun 4, 2021

chensun Jun 4, 2021

chensun commented Jun 4, 2021

chensun commented Jun 4, 2021

NikeNano commented Jun 4, 2021 •

edited

Loading

chensun commented Jun 6, 2021

NikeNano commented Jun 7, 2021 •

edited

Loading

NikeNano commented Jun 9, 2021 •

edited

Loading

chensun commented Jun 10, 2021 •

edited

Loading

chensun commented Jun 10, 2021

google-oss-robot commented Jun 10, 2021

feat(sdk): add runtime resource requests. Fixes #1956 #5447

feat(sdk): add runtime resource requests. Fixes #1956 #5447

Conversation

NikeNano commented Apr 8, 2021 • edited Loading

NikeNano commented Apr 8, 2021

NikeNano commented Apr 13, 2021

NikeNano commented Apr 22, 2021

NikeNano commented May 3, 2021

NikeNano commented May 12, 2021

capri-xiyue commented May 12, 2021 • edited Loading

capri-xiyue commented May 12, 2021

NikeNano commented May 12, 2021

Ark-kun commented May 13, 2021

NikeNano commented May 31, 2021

NikeNano commented Jun 3, 2021

tiru1930 commented Jun 3, 2021

NikeNano commented Jun 3, 2021

chensun left a comment

Choose a reason for hiding this comment

chensun Jun 4, 2021

Choose a reason for hiding this comment

chensun Jun 4, 2021

Choose a reason for hiding this comment

chensun commented Jun 4, 2021

chensun commented Jun 4, 2021

NikeNano commented Jun 4, 2021 • edited Loading

chensun commented Jun 6, 2021

NikeNano commented Jun 7, 2021 • edited Loading

NikeNano commented Jun 9, 2021 • edited Loading

chensun commented Jun 10, 2021 • edited Loading

chensun commented Jun 10, 2021

google-oss-robot commented Jun 10, 2021

NikeNano commented Apr 8, 2021 •

edited

Loading

capri-xiyue commented May 12, 2021 •

edited

Loading

NikeNano commented Jun 4, 2021 •

edited

Loading

NikeNano commented Jun 7, 2021 •

edited

Loading

NikeNano commented Jun 9, 2021 •

edited

Loading

chensun commented Jun 10, 2021 •

edited

Loading