-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: support pipeline versioning #5248
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
base: master
Are you sure you want to change the base?
Conversation
src/sagemaker/workflow/pipeline.py
Outdated
@@ -124,6 +124,7 @@ def __init__( | |||
self._event_bridge_scheduler_helper = EventBridgeSchedulerHelper( | |||
self.sagemaker_session.boto_session.client("scheduler"), | |||
) | |||
self.latest_pipeline_version_id = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just used for informational and testing purposes? Since the pipeline could be updated elsewhere which could make this inaccurate as it wouldn't be the latest version. Maybe current would be better?
if not summaries: | ||
return None | ||
else: | ||
return summaries[0].get("PipelineVersionId") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this always guaranteed to be the latest version when sort_order is None?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the default sort order is descending
"boto3>=1.35.36,<2.0", | ||
"boto3>=1.39.5,<2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we had previously relaxed the requirement to enable airflow and resolve some dependency conflicts. Any reason why this is needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we launched new apis UpdatePipelineVersion and ListPipelineVersions that were added to boto in version 1.39.5- boto/boto3@a1a8371#diff-2c623f3c6a917be56c59d43279244996836262cb1e12d9d0786c9c49eef6b43c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So currently we needed to downgrade to a lower version because airflow, which is another open source dependency depends on a lower boto version : #5245
If we do make this update , it would break customer experiences .
return self.sagemaker_session.sagemaker_client.create_pipeline(**kwargs) | ||
response = self.sagemaker_session.sagemaker_client.create_pipeline(**kwargs) | ||
return response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit : Is this change needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah no. Would you like me to revert this?
Issue #, if available:
Description of changes:
Support new pipeline versioning - https://docs.aws.amazon.com/sagemaker/latest/dg/run-pipeline.html
Testing done:
Added unit tests and integ tests
Merge Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
unique_name_from_base
to create resource names in integ tests (if appropriate)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.