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

[backend] ResourceCpuLimit, ResourceCpuRequest, ResourceMemoryLimit, and ResourceMemoryRequest are ignored #11500

Closed
mprahl opened this issue Jan 6, 2025 · 2 comments

Comments

@mprahl
Copy link
Contributor

mprahl commented Jan 6, 2025

Environment

  • How did you deploy Kubeflow Pipelines (KFP)?

I used the manifests from the master branch.

  • KFP version:

Latest from the master branch.

  • KFP SDK version:

2.11

Steps to reproduce

The Driver ignores the new fields ResourceCpuLimit, ResourceCpuRequest, ResourceMemoryLimit, and ResourceMemoryRequest. The API fields were added in this commit but the Driver code is only looking at the now deprecated fields:
c2f5649#diff-42023372ccec7209826b2d45768cbac5c1181a3a79645c3e56b9527702b29572L29

          "resources": {
            "resourceCpuLimit": "{{$.inputs.parameters['pipelinechannel--cpu_limit']}}",
            "resourceCpuRequest": "{{$.inputs.parameters['pipelinechannel--cpu_request']}}"
          }

Expected result

The new fields should be used first and then fallback to the deprecated fields.

Materials and Reference

Related SDK commit:
boarder7395@10f1dd5

Here is an example reproducer:

{
  "components": {
    "comp-generate-text": {
      "executorLabel": "exec-generate-text",
      "outputDefinitions": {
        "parameters": {
          "Output": {
            "parameterType": "STRING"
          }
        }
      }
    },
    "comp-print-text": {
      "executorLabel": "exec-print-text",
      "inputDefinitions": {
        "parameters": {
          "text": {
            "parameterType": "STRING"
          }
        }
      }
    }
  },
  "deploymentSpec": {
    "executors": {
      "exec-generate-text": {
        "container": {
          "args": [
            "--executor_input",
            "{{$}}",
            "--function_to_execute",
            "generate_text"
          ],
          "command": [
            "sh",
            "-c",
            "\nif ! [ -x \"$(command -v pip)\" ]; then\n    python3 -m ensurepip || python3 -m ensurepip --user || apt-get install python3-pip\nfi\n\nPIP_DISABLE_PIP_VERSION_CHECK=1 python3 -m pip install --quiet --no-warn-script-location 'kfp==2.11.0' '--no-deps' 'typing-extensions>=3.7.4,<5; python_version<\"3.9\"' && \"$0\" \"$@\"\n",
            "sh",
            "-ec",
            "program_path=$(mktemp -d)\n\nprintf \"%s\" \"$0\" > \"$program_path/ephemeral_component.py\"\n_KFP_RUNTIME=true python3 -m kfp.dsl.executor_main                         --component_module_path                         \"$program_path/ephemeral_component.py\"                         \"$@\"\n",
            "\nimport kfp\nfrom kfp import dsl\nfrom kfp.dsl import *\nfrom typing import *\n\ndef generate_text() -> str:\n    return \"some text from generate_text\"\n\n"
          ],
          "image": "quay.io/opendatahub/ds-pipelines-ci-executor-image:v1.0",
          "resources": {
            "resourceCpuLimit": "{{$.inputs.parameters['pipelinechannel--cpu_limit']}}",
            "resourceCpuRequest": "{{$.inputs.parameters['pipelinechannel--cpu_request']}}"
          }
        }
      },
      "exec-print-text": {
        "container": {
          "args": [
            "--executor_input",
            "{{$}}",
            "--function_to_execute",
            "print_text"
          ],
          "command": [
            "sh",
            "-c",
            "\nif ! [ -x \"$(command -v pip)\" ]; then\n    python3 -m ensurepip || python3 -m ensurepip --user || apt-get install python3-pip\nfi\n\nPIP_DISABLE_PIP_VERSION_CHECK=1 python3 -m pip install --quiet --no-warn-script-location 'kfp==2.11.0' '--no-deps' 'typing-extensions>=3.7.4,<5; python_version<\"3.9\"' && \"$0\" \"$@\"\n",
            "sh",
            "-ec",
            "program_path=$(mktemp -d)\n\nprintf \"%s\" \"$0\" > \"$program_path/ephemeral_component.py\"\n_KFP_RUNTIME=true python3 -m kfp.dsl.executor_main                         --component_module_path                         \"$program_path/ephemeral_component.py\"                         \"$@\"\n",
            "\nimport kfp\nfrom kfp import dsl\nfrom kfp.dsl import *\nfrom typing import *\n\ndef print_text(text: str):\n    print(text)\n\n"
          ],
          "image": "quay.io/opendatahub/ds-pipelines-ci-executor-image:v1.0"
        }
      }
    }
  },
  "pipelineInfo": {
    "description": "A simple intro pipeline",
    "name": "hello-world"
  },
  "root": {
    "dag": {
      "tasks": {
        "generate-text": {
          "cachingOptions": {},
          "componentRef": {
            "name": "comp-generate-text"
          },
          "inputs": {
            "parameters": {
              "cpu_limit": {
                "runtimeValue": {
                  "constant": "{{$.inputs.parameters['pipelinechannel--cpu_limit']}}"
                }
              },
              "cpu_request": {
                "runtimeValue": {
                  "constant": "{{$.inputs.parameters['pipelinechannel--cpu_request']}}"
                }
              },
              "pipelinechannel--cpu_limit": {
                "componentInputParameter": "cpu_limit"
              },
              "pipelinechannel--cpu_request": {
                "componentInputParameter": "cpu_request"
              }
            }
          },
          "taskInfo": {
            "name": "generate-text"
          }
        },
        "print-text": {
          "cachingOptions": {},
          "componentRef": {
            "name": "comp-print-text"
          },
          "dependentTasks": [
            "generate-text"
          ],
          "inputs": {
            "parameters": {
              "text": {
                "taskOutputParameter": {
                  "outputParameterKey": "Output",
                  "producerTask": "generate-text"
                }
              }
            }
          },
          "taskInfo": {
            "name": "print-text"
          }
        }
      }
    },
    "inputDefinitions": {
      "parameters": {
        "cpu_limit": {
          "defaultValue": "400m",
          "isOptional": true,
          "parameterType": "STRING"
        },
        "cpu_request": {
          "defaultValue": "200m",
          "isOptional": true,
          "parameterType": "STRING"
        }
      }
    }
  },
  "schemaVersion": "2.1.0",
  "sdkVersion": "kfp-2.11.0"
}

Impacted by this bug? Give it a 👍.

@mprahl
Copy link
Contributor Author

mprahl commented Jan 6, 2025

/assign @mprahl

mprahl added a commit to mprahl/pipelines that referenced this issue Jan 6, 2025
The API introduced new fields prefixed with Resource (e.g.
ResourceCpuLimit) to replace the old fields without the prefix. The
Driver hadn't been updated to honor those fields but the SDK started
using them which led to unexpected behavior.

The Driver now honors both fields but prioritizes the new fields. The
SDK now only sets the new fields.

The outcome is that resource limits/requests can now use input
parameters.

Note that pipeline_spec_builder.py was doing some validation on the
limits/requests being set, but that's already handled in the user facing
method (e.g. set_cpu_limit).

Resolves:
kubeflow#11500

Signed-off-by: mprahl <[email protected]>
mprahl added a commit to mprahl/pipelines that referenced this issue Jan 6, 2025
The API introduced new fields prefixed with Resource (e.g.
ResourceCpuLimit) to replace the old fields without the prefix. The
Driver hadn't been updated to honor those fields but the SDK started
using them which led to unexpected behavior.

The Driver now honors both fields but prioritizes the new fields. The
SDK now only sets the new fields.

The outcome is that resource limits/requests can now use input
parameters.

Note that pipeline_spec_builder.py was doing some validation on the
limits/requests being set, but that's already handled in the user facing
method (e.g. set_cpu_limit).

Resolves:
kubeflow#11500

Signed-off-by: mprahl <[email protected]>
mprahl added a commit to mprahl/pipelines that referenced this issue Jan 6, 2025
The API introduced new fields prefixed with Resource (e.g.
ResourceCpuLimit) to replace the old fields without the prefix. The
Driver hadn't been updated to honor those fields but the SDK started
using them which led to unexpected behavior.

The Driver now honors both fields but prioritizes the new fields. The
SDK now only sets the new fields.

The outcome is that resource limits/requests can now use input
parameters.

Note that pipeline_spec_builder.py was doing some validation on the
limits/requests being set, but that's already handled in the user facing
method (e.g. set_cpu_limit).

Resolves:
kubeflow#11500

Signed-off-by: mprahl <[email protected]>
mprahl added a commit to mprahl/pipelines that referenced this issue Jan 6, 2025
The API introduced new fields prefixed with Resource (e.g.
ResourceCpuLimit) to replace the old fields without the prefix. The
Driver hadn't been updated to honor those fields but the SDK started
using them which led to unexpected behavior.

The Driver now honors both fields but prioritizes the new fields. The
SDK now only sets the new fields.

The outcome is that resource limits/requests can now use input
parameters.

Note that pipeline_spec_builder.py was doing some validation on the
limits/requests being set, but that's already handled in the user facing
method (e.g. set_cpu_limit).

Resolves:
kubeflow#11500

Signed-off-by: mprahl <[email protected]>
@mprahl
Copy link
Contributor Author

mprahl commented Jan 6, 2025

Here is a Slack thread discussing the issue on the SDK side:
https://cloud-native.slack.com/archives/C073N7BMLB1/p1735746170303549

mprahl added a commit to mprahl/pipelines that referenced this issue Jan 6, 2025
The API introduced new fields prefixed with Resource (e.g.
ResourceCpuLimit) to replace the old fields without the prefix. The
Driver hadn't been updated to honor those fields but the SDK started
using them which led to unexpected behavior.

The Driver now honors both fields but prioritizes the new fields. The
SDK now only sets the new fields.

The outcome is that resource limits/requests can now use input
parameters.

Note that pipeline_spec_builder.py was doing some validation on the
limits/requests being set, but that's already handled in the user facing
method (e.g. set_cpu_limit).

Resolves:
kubeflow#11500

Signed-off-by: mprahl <[email protected]>
mprahl added a commit to mprahl/pipelines that referenced this issue Jan 9, 2025
The API introduced new fields prefixed with Resource (e.g.
ResourceCpuLimit) to replace the old fields without the prefix. The
Driver hadn't been updated to honor those fields but the SDK started
using them which led to unexpected behavior.

The Driver now honors both fields but prioritizes the new fields. The
SDK now only sets the new fields.

The outcome is that resource limits/requests can now use input
parameters.

Note that pipeline_spec_builder.py was doing some validation on the
limits/requests being set, but that's already handled in the user facing
method (e.g. set_cpu_limit).

Resolves:
kubeflow#11500

Signed-off-by: mprahl <[email protected]>
mprahl added a commit to mprahl/pipelines that referenced this issue Jan 9, 2025
The API introduced new fields prefixed with Resource (e.g.
ResourceCpuLimit) to replace the old fields without the prefix. The
Driver hadn't been updated to honor those fields but the SDK started
using them which led to unexpected behavior.

The Driver now honors both fields but prioritizes the new fields. The
SDK now only sets the new fields.

The outcome is that resource limits/requests can now use input
parameters.

Note that pipeline_spec_builder.py was doing some validation on the
limits/requests being set, but that's already handled in the user facing
method (e.g. set_cpu_limit).

Resolves:
kubeflow#11500

Signed-off-by: mprahl <[email protected]>
rimolive pushed a commit to rimolive/data-science-pipelines that referenced this issue Jan 20, 2025
…ubeflow#11501)

The API introduced new fields prefixed with Resource (e.g.
ResourceCpuLimit) to replace the old fields without the prefix. The
Driver hadn't been updated to honor those fields but the SDK started
using them which led to unexpected behavior.

The Driver now honors both fields but prioritizes the new fields. The
SDK now only sets the new fields.

The outcome is that resource limits/requests can now use input
parameters.

Note that pipeline_spec_builder.py was doing some validation on the
limits/requests being set, but that's already handled in the user facing
method (e.g. set_cpu_limit).

Resolves:
kubeflow#11500

Signed-off-by: mprahl <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant