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

Implement a Sagemaker DeleteModelOperator and Delete model hook. #21673

Merged
merged 5 commits into from
Feb 27, 2022

Conversation

hsrocks
Copy link
Contributor

@hsrocks hsrocks commented Feb 18, 2022

This PR implements operator for Sagemaker operator and hooks to delete the Sagemaker model


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@boring-cyborg boring-cyborg bot added area:providers provider:amazon-aws AWS/Amazon - related issues labels Feb 18, 2022
@eladkal
Copy link
Contributor

eladkal commented Feb 19, 2022

@hsrocks
Copy link
Contributor Author

hsrocks commented Feb 19, 2022

Can you please add docs and example dag? https://github.com/apache/airflow/tree/main/airflow/providers/amazon/aws/example_dags https://github.com/apache/airflow/tree/main/docs/apache-airflow-providers-amazon/operators

Sure! Valid point @eladkal . Can see its missing for sagemaker altogether. Will do it . The failed test '
Tests / MySQL5.7, Py3.7: Always Integration Providers (pull_request) ' is not related to changes. Can you please suggest @eladkal ?

@potiuk
Copy link
Member

potiuk commented Feb 19, 2022

Can you please add docs and example dag? https://github.com/apache/airflow/tree/main/airflow/providers/amazon/aws/example_dags https://github.com/apache/airflow/tree/main/docs/apache-airflow-providers-amazon/operators

Sure! Valid point @eladkal . Can see its missing for sagemaker altogether. Will do it . The failed test ' Tests / MySQL5.7, Py3.7: Always Integration Providers (pull_request) ' is not related to changes. Can you please suggest @eladkal ?

It could be flaky failure. Try again.

@eladkal
Copy link
Contributor

eladkal commented Feb 19, 2022

Yep probably not related. Once you will add the docs and example I'll review

@hsrocks hsrocks marked this pull request as draft February 19, 2022 18:10
@hsrocks hsrocks force-pushed the sagemaker_delete_model_operator branch from 0f04379 to 7c43d25 Compare February 21, 2022 09:27
@hsrocks hsrocks marked this pull request as ready for review February 21, 2022 09:30
@hsrocks
Copy link
Contributor Author

hsrocks commented Feb 21, 2022

eladkal

Can you please add docs and example dag? https://github.com/apache/airflow/tree/main/airflow/providers/amazon/aws/example_dags https://github.com/apache/airflow/tree/main/docs/apache-airflow-providers-amazon/operators

Sure! Valid point @eladkal . Can see its missing for sagemaker altogether. Will do it . The failed test ' Tests / MySQL5.7, Py3.7: Always Integration Providers (pull_request) ' is not related to changes. Can you please suggest @eladkal ?

eladkal

can you please check the code and share your feedback?

@eladkal eladkal self-requested a review February 23, 2022 17:47
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ferruzzi will you be able to also review?

airflow/providers/amazon/aws/operators/sagemaker.py Outdated Show resolved Hide resolved
airflow/providers/amazon/aws/operators/sagemaker.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ferruzzi ferruzzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments.

@hsrocks hsrocks force-pushed the sagemaker_delete_model_operator branch 3 times, most recently from 1bc8949 to 5a874d6 Compare February 24, 2022 16:18
@hsrocks hsrocks marked this pull request as draft February 24, 2022 16:22
@hsrocks hsrocks force-pushed the sagemaker_delete_model_operator branch from 5a874d6 to 37d1a29 Compare February 24, 2022 16:26
@hsrocks hsrocks marked this pull request as ready for review February 24, 2022 16:40
@hsrocks hsrocks marked this pull request as draft February 24, 2022 17:22
@hsrocks hsrocks force-pushed the sagemaker_delete_model_operator branch 2 times, most recently from f7765a4 to d4bf264 Compare February 24, 2022 19:23
@ferruzzi
Copy link
Contributor

Looks like my points have been addressed other than that new question about the indenting of the START/END flag in the sample DAG, which may or may not be a concern. Thanks for the changes.

@hsrocks
Copy link
Contributor Author

hsrocks commented Feb 25, 2022

Looks like my points have been addressed other than that new question about the indenting of the START/END flag in the sample DAG, which may or may not be a concern. Thanks for the changes.

Thanks a lot for review :) No it does not failed for me.

@hsrocks hsrocks marked this pull request as ready for review February 25, 2022 01:14
@hsrocks hsrocks requested a review from eladkal February 25, 2022 01:23
@ferruzzi
Copy link
Contributor

ferruzzi commented Feb 25, 2022

image
@hsrocks The failing test doesn't appear to be related. The file wasn't touched by this PR and the line that is failing hasn't been touched in months, but @ashb was in that file yesterday and may have more info.

@ashb
Copy link
Member

ashb commented Feb 25, 2022

If in doubt rebase. In this case it will help as it has been fixed on main.

@hsrocks
Copy link
Contributor Author

hsrocks commented Feb 25, 2022

If in doubt rebase. In this case it will help as it has been fixed on main.

Thanks @ashb ! I already did it twice before but let me do it again :) Thanks for the help!!

@hsrocks hsrocks force-pushed the sagemaker_delete_model_operator branch from d4bf264 to 161b968 Compare February 25, 2022 19:24
@hsrocks hsrocks force-pushed the sagemaker_delete_model_operator branch from 161b968 to 06aac01 Compare February 27, 2022 11:12
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Feb 27, 2022
@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@eladkal eladkal merged commit cb24ee9 into apache:main Feb 27, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Feb 27, 2022

Awesome work, congrats on your first merged pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers okay to merge It's ok to merge this PR as it does not require more tests provider:amazon-aws AWS/Amazon - related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants