fix(backend): Fix enable_caching issues when handling PVC creation/deletion #11411

rimolive · 2024-11-26T20:42:58Z

Description of your changes:
Resolves #10188

When setting enable_caching=false, KFP Driver still tries to create the cache entry. Added a condition to ensure cache entry will only get created if enable_caching=true.

Checklist:

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

backend/src/v2/driver/driver.go

DharmitD · 2024-11-26T21:22:45Z

backend/src/v2/driver/driver.go

-	err = createCache(ctx, createdExecution, opts, taskStartedTime, fingerPrint, cacheClient)
-	if err != nil {
-		return "", createdExecution, pb.Execution_FAILED, fmt.Errorf("failed to create cache entrty for create pvc: %w", err)
+	if opts.Task.GetCachingOptions().GetEnableCache() && ecfg.CachedMLMDExecutionID != "" {


ecfg.CachedMLMDExecutionID != "" condition might possibly not be necessary here, as creating a PVC doesn't always depend on a cached execution ID. The cached execution ID might not exist at the time of PVC creation.
Unlike here, as deleting a PVC without a valid cached execution ID could result in errors or unintended behavior (e.g., trying to delete a PVC that doesn’t exist or isn't linked to the cache).

you are right about createPVC, but it seems to me like this won't have the intended result for deletepvc either

Unlike here, as deleting a PVC without a valid cached execution ID could result in errors or unintended behavior (e.g., trying to delete a PVC that doesn’t exist or isn't linked to the cache).

when would we ever hit this in either case?

this exact condition is checked here, and this results in an early exit.

Regardless, how can ecfg.CachedMLMDExecutionID != "" ever be true if we never create a cache outside this condition?

@rimolive did you test to see if deletepvc is even getting cached?

@HumairAK Yes, tested with the same pipeline but creating/deleting the PVC and with cache enabled. The cache fingerprint is generated in both tasks, so maybe this condition should also apply to deletePVC.

hbelmiro

/lgtm

VaniHaripriya · 2024-12-04T21:47:02Z

/lgtm

mprahl

Could you please add a test?

It seems that samples/v2/pipeline_with_volume.py already tests it with the cache enabled. It should be simple enough to copy this test but with the cache disabled?

rimolive · 2025-01-14T13:13:55Z

Could you please add a test?

I'm afraid it is not possible because it's a special condition when you create a run directly from the SDK:

client.create_run_from_pipeline_func(test_pipeline, arguments={}, enable_caching=False)

@mprahl Sample test does not interact with KFP that way, it only compiles the sample and submit as-is to KFP. https://github.com/kubeflow/pipelines/blob/master/samples/v2/sample_test.py#L92

Personally, I think duplicating this code just to cover one single case is overengineering. I can take a look at other options, but I'm open for other suggestions.

mprahl · 2025-01-14T14:27:49Z

Could you please add a test?
I'm afraid it is not possible because it's a special condition when you create a run directly from the SDK:
client.create_run_from_pipeline_func(test_pipeline, arguments={}, enable_caching=False)
@mprahl Sample test does not interact with KFP that way, it only compiles the sample and submit as-is to KFP. https://github.com/kubeflow/pipelines/blob/master/samples/v2/sample_test.py#L92

Personally, I think duplicating this code just to cover one single case is overengineering. I can take a look at other options, but I'm open for other suggestions.

pipeline_with_volume

I disagree that this is "overengineering". Since this used to work before and was broken and not caught (i.e. regression) in a newer release, that signals to me that the test coverage is not sufficient.

Submitting the job with enable_caching=False as you showed in your sample is an override of the compiled YAML. You can explicitly disable the cache through the SDK. Here is a diff of the existing sample that disables caching. I'm not suggesting to change the existing test but to add an additional one with this diff.

diff --git a/samples/v2/pipeline_with_volume.py b/samples/v2/pipeline_with_volume.py
index c1a8b1035..216e0947a 100644
--- a/samples/v2/pipeline_with_volume.py
+++ b/samples/v2/pipeline_with_volume.py
@@ -41,10 +41,10 @@ def pipeline_with_volume():
         access_modes=['ReadWriteOnce'],
         size='5Mi',
         storage_class_name='standard',
-    )
+    ).set_caching_options(False)
 
-    task1 = producer()
-    task2 = consumer().after(task1)
+    task1 = producer().set_caching_options(False)
+    task2 = consumer().set_caching_options(False).after(task1)
 
     kubernetes.mount_pvc(
         task1,
@@ -58,11 +58,11 @@ def pipeline_with_volume():
     )
 
     delete_pvc1 = kubernetes.DeletePVC(
-        pvc_name=pvc1.outputs['name']).after(task2)
+        pvc_name=pvc1.outputs['name']).set_caching_options(False).after(task2)
 
 if __name__ == '__main__':
     # execute only if run as a script
     from kfp import compiler
     compiler.Compiler().compile(
         pipeline_func=pipeline_with_volume,
-        package_path='pipeline_with_volume.json')
\ No newline at end of file
+        package_path='pipeline_with_volume.yaml')
\ No newline at end of file

mprahl · 2025-01-15T16:37:26Z

samples/v2/pipeline_with_volume_no_cache.py

+        mount_path='/data',
+    )
+
+    delete_pvc1 = kubernetes.DeletePVC(


Does this need to also have caching disabled for the deletePvc function test coverage?

Good catch, as the Driver changes apply to both functions. I'm adding it to the test.

mprahl · 2025-01-15T16:41:31Z

samples/v2/Makefile

@@ -16,9 +16,9 @@
 .PHONY: all
 all:
 	@for f in $$(find . -name "*.py"); do \
-		compiled="$${f%.*}_pipeline.json"; \
+		compiled="$${f%.*}_pipeline.yaml"; \


I think you'll need to update the clean Make target as well since it looks for all JSON files still.

mprahl · 2025-01-15T16:42:01Z

samples/v2/pipeline_with_volume_no_cache.py

+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+"""Pipeline with volume creation, mount and deletion in v2 engine pipeline."""


Suggested change

"""Pipeline with volume creation, mount and deletion in v2 engine pipeline."""

"""Pipeline with no caching on volume creation, mount and deletion in v2 engine pipeline."""

Signed-off-by: Ricardo M. Oliveira <rmartine@redhat.com>

mprahl

/lgtm

rimolive · 2025-01-15T19:27:13Z

@HumairAK Can you PTAL?

HumairAK · 2025-01-16T15:31:18Z

/approve

google-oss-prow · 2025-01-16T15:31:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HumairAK, mprahl

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:

~~backend/OWNERS~~ [HumairAK]
~~samples/OWNERS~~ [HumairAK]

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 requested review from hbelmiro and HumairAK November 26, 2024 20:43

google-oss-prow bot added the size/S label Nov 26, 2024

DharmitD reviewed Nov 26, 2024

View reviewed changes

backend/src/v2/driver/driver.go Outdated Show resolved Hide resolved

DharmitD reviewed Nov 26, 2024

View reviewed changes

backend/src/v2/driver/driver.go Outdated Show resolved Hide resolved

github-actions bot added the ci-passed All CI tests on a pull request have passed label Nov 26, 2024

DharmitD reviewed Nov 26, 2024

View reviewed changes

rimolive force-pushed the caching_error branch from bf69940 to 400d108 Compare November 27, 2024 12:05

github-actions bot added ci-passed All CI tests on a pull request have passed and removed ci-passed All CI tests on a pull request have passed labels Nov 27, 2024

rimolive force-pushed the caching_error branch from 400d108 to 5c5d99a Compare November 27, 2024 14:48

github-actions bot added ci-passed All CI tests on a pull request have passed and removed ci-passed All CI tests on a pull request have passed labels Nov 27, 2024

hbelmiro reviewed Nov 28, 2024

View reviewed changes

google-oss-prow bot assigned hbelmiro Nov 28, 2024

google-oss-prow bot added the lgtm label Nov 28, 2024

google-oss-prow bot assigned VaniHaripriya Dec 4, 2024

mprahl suggested changes Jan 9, 2025

View reviewed changes

HumairAK mentioned this pull request Jan 10, 2025

nominate approver & reviewer for backend #11507

Merged

rimolive force-pushed the caching_error branch from 5c5d99a to 7ebe489 Compare January 15, 2025 14:38

google-oss-prow bot added size/L and removed lgtm size/S labels Jan 15, 2025

mprahl reviewed Jan 15, 2025

View reviewed changes

rimolive force-pushed the caching_error branch from 7ebe489 to 0230152 Compare January 15, 2025 16:40

mprahl reviewed Jan 15, 2025

View reviewed changes

rimolive force-pushed the caching_error branch from 0230152 to 06e8466 Compare January 15, 2025 16:51

Fix enable_caching issues when handling PVC creation/deletion

ac2c533

Signed-off-by: Ricardo M. Oliveira <rmartine@redhat.com>

rimolive force-pushed the caching_error branch from 06e8466 to ac2c533 Compare January 15, 2025 16:57

mprahl approved these changes Jan 15, 2025

View reviewed changes

google-oss-prow bot assigned mprahl Jan 15, 2025

google-oss-prow bot added the lgtm label Jan 15, 2025

google-oss-prow bot added the approved label Jan 16, 2025

google-oss-prow bot merged commit 027ca8b into kubeflow:master Jan 16, 2025
31 of 32 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(backend): Fix enable_caching issues when handling PVC creation/deletion #11411

fix(backend): Fix enable_caching issues when handling PVC creation/deletion #11411

rimolive commented Nov 26, 2024

DharmitD Nov 26, 2024

HumairAK Dec 3, 2024

rimolive Dec 4, 2024

hbelmiro left a comment

VaniHaripriya commented Dec 4, 2024

mprahl left a comment

rimolive commented Jan 14, 2025 •

edited

Loading

mprahl commented Jan 14, 2025

mprahl Jan 15, 2025

rimolive Jan 15, 2025

mprahl Jan 15, 2025

mprahl Jan 15, 2025

mprahl left a comment

rimolive commented Jan 15, 2025

HumairAK commented Jan 16, 2025

google-oss-prow bot commented Jan 16, 2025

	"""Pipeline with volume creation, mount and deletion in v2 engine pipeline."""
	"""Pipeline with no caching on volume creation, mount and deletion in v2 engine pipeline."""

fix(backend): Fix enable_caching issues when handling PVC creation/deletion #11411

fix(backend): Fix enable_caching issues when handling PVC creation/deletion #11411

Conversation

rimolive commented Nov 26, 2024

DharmitD Nov 26, 2024

Choose a reason for hiding this comment

HumairAK Dec 3, 2024

Choose a reason for hiding this comment

rimolive Dec 4, 2024

Choose a reason for hiding this comment

hbelmiro left a comment

Choose a reason for hiding this comment

VaniHaripriya commented Dec 4, 2024

mprahl left a comment

Choose a reason for hiding this comment

rimolive commented Jan 14, 2025 • edited Loading

mprahl commented Jan 14, 2025

mprahl Jan 15, 2025

Choose a reason for hiding this comment

rimolive Jan 15, 2025

Choose a reason for hiding this comment

mprahl Jan 15, 2025

Choose a reason for hiding this comment

mprahl Jan 15, 2025

Choose a reason for hiding this comment

mprahl left a comment

Choose a reason for hiding this comment

rimolive commented Jan 15, 2025

HumairAK commented Jan 16, 2025

google-oss-prow bot commented Jan 16, 2025

rimolive commented Jan 14, 2025 •

edited

Loading