fix(local): resolve PIPELINE_JOB_RESOURCE_NAME, CREATE_TIME, and SCHEDULE_TIME placeholders#13426
Conversation
Add PIPELINE_JOB_RESOURCE_NAME_PLACEHOLDER, PIPELINE_JOB_CREATE_TIME_UTC_PLACEHOLDER, and PIPELINE_JOB_SCHEDULE_TIME_UTC_PLACEHOLDER to the placeholder resolution map in local execution. Timestamps are generated as timezone-aware UTC in ISO 8601 format. Signed-off-by: Shreeharsh Shinde <shindeshreeharsh157@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @shreeharshshinde. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for additional local KFP placeholders, including pipeline job resource name and UTC timestamps for create/schedule time, with accompanying unit tests.
Changes:
- Add new placeholder mappings for pipeline job resource name and UTC create/schedule timestamps.
- Add unit tests to cover the new placeholder keys and validate timestamp formatting.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| sdk/python/kfp/local/placeholder_utils.py | Adds placeholder mappings for pipeline job resource name and UTC create/schedule timestamp values. |
| sdk/python/kfp/local/placeholder_utils_test.py | Adds tests for the new placeholders, including regex validation for UTC timestamp formats. |
| @@ -369,8 +374,14 @@ def resolve_individual_placeholder( | |||
| json.dumps(executor_input_dict), | |||
| dsl.PIPELINE_JOB_NAME_PLACEHOLDER: | |||
| pipeline_resource_name, | |||
| dsl.PIPELINE_JOB_RESOURCE_NAME_PLACEHOLDER: | |||
| pipeline_resource_name, | |||
| dsl.PIPELINE_JOB_ID_PLACEHOLDER: | |||
| pipeline_job_id, | |||
| dsl.PIPELINE_JOB_CREATE_TIME_UTC_PLACEHOLDER: | |||
| utc_timestamp, | |||
| dsl.PIPELINE_JOB_SCHEDULE_TIME_UTC_PLACEHOLDER: | |||
| utc_timestamp, | |||
| def test_pipeline_job_create_time_utc_placeholder(self): | ||
| """Test that PIPELINE_JOB_CREATE_TIME_UTC_PLACEHOLDER is replaced with a valid UTC timestamp.""" | ||
| actual = placeholder_utils.resolve_individual_placeholder( | ||
| element='{{$.pipeline_job_create_time_utc}}', | ||
| executor_input_dict=EXECUTOR_INPUT_DICT, | ||
| pipeline_resource_name='my-pipeline-2023-10-10-13-32-59-420710', | ||
| task_resource_name='comp', | ||
| pipeline_root='/foo/bar/my-pipeline-2023-10-10-13-32-59-420710', | ||
| pipeline_job_id='123456789', | ||
| pipeline_task_id='987654321', | ||
| ) | ||
| # Verify that the result is a non-empty string in ISO 8601 format (YYYY-MM-DDTHH:MM:SS.sssZ) | ||
| import re | ||
| iso8601_pattern = r'\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d+Z' | ||
| self.assertRegex(actual, iso8601_pattern) |
| def test_pipeline_job_schedule_time_utc_placeholder(self): | ||
| """Test that PIPELINE_JOB_SCHEDULE_TIME_UTC_PLACEHOLDER is replaced with a valid UTC timestamp.""" | ||
| actual = placeholder_utils.resolve_individual_placeholder( | ||
| element='{{$.pipeline_job_schedule_time_utc}}', | ||
| executor_input_dict=EXECUTOR_INPUT_DICT, | ||
| pipeline_resource_name='my-pipeline-2023-10-10-13-32-59-420710', | ||
| task_resource_name='comp', | ||
| pipeline_root='/foo/bar/my-pipeline-2023-10-10-13-32-59-420710', | ||
| pipeline_job_id='123456789', | ||
| pipeline_task_id='987654321', | ||
| ) | ||
| # Verify that the result is a non-empty string in ISO 8601 format (YYYY-MM-DDTHH:MM:SS.sssZ) | ||
| import re | ||
| iso8601_pattern = r'\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d+Z' | ||
| self.assertRegex(actual, iso8601_pattern) |
Ensure consistent timestamp values across different placeholders by passing a single through the resolution hierarchy. Previously, individual resolution functions generated their own timestamps, which could lead to discrepancies between related placeholders. Updated , , , and to accept as an argument. Added corresponding unit tests to verify timestamp consistency.fix(local): resolve missing pipeline job placeholders at runtime Add PIPELINE_JOB_RESOURCE_NAME_PLACEHOLDER, PIPELINE_JOB_CREATE_TIME_UTC_PLACEHOLDER, and PIPELINE_JOB_SCHEDULE_TIME_UTC_PLACEHOLDER to the placeholder resolution map in local execution. Timestamps are generated as timezone-aware UTC in ISO 8601 format. Signed-off-by: Shreeharsh Shinde <shindeshreeharsh157@gmail.com>
|
All three Copilot comments were valid and are now fixed: High — timestamp stability: utc_timestamp is now generated once in replace_placeholders Low — import re inside test method: Moved to module scope alongside the new import Medium — regex not anchored / no same-value assertion: Pattern is now ^...$. |
| import datetime | ||
| import json | ||
| import os | ||
| import re |
| unique_task_id = make_random_id() | ||
| utc_timestamp = datetime.datetime.now( | ||
| datetime.timezone.utc).strftime('%Y-%m-%dT%H:%M:%S.%fZ') |
| dsl.PIPELINE_JOB_CREATE_TIME_UTC_PLACEHOLDER: | ||
| utc_timestamp, | ||
| dsl.PIPELINE_JOB_SCHEDULE_TIME_UTC_PLACEHOLDER: | ||
| utc_timestamp, |
| dsl.PIPELINE_JOB_NAME_PLACEHOLDER: | ||
| pipeline_resource_name, | ||
| dsl.PIPELINE_JOB_RESOURCE_NAME_PLACEHOLDER: | ||
| pipeline_resource_name, |
Problem
Fixes #13424.
Three
dslplaceholders were silently left unresolved when running pipelines locally viakfp.local. Components using these placeholders received the raw placeholder string instead of the actual runtime value.Affected placeholders
dsl.PIPELINE_JOB_RESOURCE_NAME_PLACEHOLDERdsl.PIPELINE_JOB_CREATE_TIME_UTC_PLACEHOLDERdsl.PIPELINE_JOB_SCHEDULE_TIME_UTC_PLACEHOLDERAll other job/task placeholders were already resolved correctly:
PIPELINE_JOB_NAMEPIPELINE_JOB_IDPIPELINE_TASK_NAMEPIPELINE_TASK_IDPIPELINE_ROOTThese three placeholders were simply missing from the local placeholder resolution map.
Root Cause
resolve_individual_placeholder()in:maintains a
PLACEHOLDERSmapping that resolves placeholder strings to runtime values.Although the following placeholders were defined in
kfp.dsl, they were never added to this mapping:PIPELINE_JOB_RESOURCE_NAME_PLACEHOLDERPIPELINE_JOB_CREATE_TIME_UTC_PLACEHOLDERPIPELINE_JOB_SCHEDULE_TIME_UTC_PLACEHOLDERAs a result, local execution returned unresolved raw placeholder strings.
Changes
sdk/python/kfp/local/placeholder_utils.pyAdded support for the following placeholders:
PIPELINE_JOB_RESOURCE_NAME_PLACEHOLDERpipeline_resource_namePIPELINE_JOB_CREATE_TIME_UTC_PLACEHOLDERPIPELINE_JOB_SCHEDULE_TIME_UTC_PLACEHOLDERTests
sdk/python/kfp/local/placeholder_utils_test.pyAdded coverage for the newly supported placeholders:
{{$.pipeline_job_resource_name}}totest_constant_placeholderstest_pipeline_job_create_time_utc_placeholdertest_pipeline_job_schedule_time_utc_placeholderThe timestamp tests validate that resolved values match the expected ISO 8601 UTC format.