pipelines icon indicating copy to clipboard operation
pipelines copied to clipboard

WIP: Add an env var and CLI option to disable default caching

Open DharmitD opened this issue 1 year ago • 1 comments

Description of your changes: Add an env var and CLI option to disable default caching, such that the CLI flag setting will take precedence over the environment variable setting. Here's how it works:

If the --disable-caching flag is provided when running the command, it directly controls the disable_caching value. If the flag is set (i.e., --disable-caching is used), disable_caching will be True. If the flag is not set, disable_caching will be None.

The environment variable KFP_DISABLE_CACHING is only used if the disable_caching flag is not provided.

Example Scenarios Scenario 1: CLI Flag is Provided kfp dsl compile --py cache.py --output output.yaml --disable-caching Result: Caching is disabled, regardless of the environment variable. Scenario 2: CLI Flag is Not Provided, Environment Variable is Set export KFP_DISABLE_CACHING=true && kfp dsl compile --py cache.py --output output.yaml Result: Caching is disabled because the environment variable is set to 'true'. Scenario 3: Neither CLI Flag Nor Environment Variable is Set kfp dsl compile --py cache.py --output output.yaml Result: Caching is enabled, as neither the flag nor the environment variable is set to disable it.

This logic provides flexibility while ensuring that the user’s explicit command-line instructions (the CLI flag) take precedence over environment settings.

Checklist:

DharmitD avatar Aug 27 '24 19:08 DharmitD

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign connor-mccarthy for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

google-oss-prow[bot] avatar Aug 27 '24 19:08 google-oss-prow[bot]

Ready for review. cc: @gregsheremeta @zijianjoy @chensun

DharmitD avatar Aug 29 '24 20:08 DharmitD

/retest test-run-all-gcpc-modules

DharmitD avatar Aug 30 '24 12:08 DharmitD

@DharmitD: The /retest command does not accept any targets. The following commands are available to trigger required jobs:

  • /test kubeflow-pipelines-integration-v2
  • /test test-run-all-gcpc-modules

Use /test all to run the following jobs that were automatically triggered:

  • test-run-all-gcpc-modules

In response to this:

/retest test-run-all-gcpc-modules

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

google-oss-prow[bot] avatar Aug 30 '24 12:08 google-oss-prow[bot]

Environment variable values should align with the conventions used by other Python libraries. Typically, 0 is used to represent false, while any other integer is treated as true. Although some approaches allow for more variation in representing true, such as accepting [1, y, yes, true], it's best to maintain consistency by using only 0 for false and any other integer for true.

diegolovison avatar Aug 30 '24 12:08 diegolovison

To @diegolovison 's point, I also noticed this in the compiler today. There's a strong argument to be made for keeping the user interface consistent.

https://github.com/kubeflow/pipelines/blob/1c7954de261b22fa93905cf11b6b280fdf19c320/sdk/python/kfp/cli/compile_.py#L131-L135

@DharmitD , how difficult would it be to rework this using a Feature flag?

gregsheremeta avatar Sep 03 '24 20:09 gregsheremeta

Closing as this has been covered in https://github.com/kubeflow/pipelines/pull/11222

DharmitD avatar Sep 20 '24 04:09 DharmitD