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.

Sign up for GitHub

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

Jump to bottom

Enable Auto-incrementing Transform job name in SageMakerTransformOperator #25263

Merged
merged 11 commits into from
Aug 2, 2022
Merged

Enable Auto-incrementing Transform job name in SageMakerTransformOperator #25263

merged 11 commits into from
Aug 2, 2022

Conversation

celeriev
Copy link
Contributor

@celeriev celeriev commented Jul 24, 2022 •
edited
Loading

Add an option to check if transform job name already exists in SageMakerTransformOperator and increment it if it happen

closes: #21941

@boring-cyborg boring-cyborg bot added area:providers provider:amazon-aws AWS/Amazon - related issues labels Jul 24, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Jul 24, 2022

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

  • Pay attention to the quality of your code (flake8, mypy and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@celeriev celeriev changed the title Draft: Option to Auto Increment Transform job name in SageMakerTransformOperator Option to Auto Increment Transform job name in SageMakerTransformOperator Jul 24, 2022
@celeriev celeriev changed the title Option to Auto Increment Transform job name in SageMakerTransformOperator Enable Auto-incrementing Transform job name in SageMakerTransformOperator Jul 24, 2022
@potiuk
Copy link
Member

potiuk commented Jul 25, 2022

Rebased to account for Werkzeug issue

@potiuk
Copy link
Member

potiuk commented Jul 26, 2022

@ferruzzi @vincbeck @o-nikolas ? Can you take a look please?

ferruzzi
ferruzzi requested changes Jul 26, 2022
View reviewed changes
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.

Looks like basically a copy/paste of the same feature in the SageMakerTrainingOperator, yeah?

airflow/providers/amazon/aws/operators/sagemaker.py Outdated
@@ -419,6 +426,8 @@ def __init__(
self.wait_for_completion = wait_for_completion
self.check_interval = check_interval
self.max_ingestion_time = max_ingestion_time
self.check_if_job_exists = check_if_job_exists
self.action_if_job_exists = action_if_job_exists
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a check here to make sure the value is something you support. See the same feature on L658

@celeriev
Copy link
Contributor Author

Looks like basically a copy/paste of the same feature in the SageMakerTrainingOperator, yeah?

@ferruzzi yes the logic is the same. I'll make a suggestion with a generic method for this inside SageMakerBaseOperator

@ferruzzi
Copy link
Contributor

Yup, cool. Once you make that change I suggested, I'm good to go.

@celeriev
Copy link
Contributor Author

celeriev commented Aug 1, 2022

@ferruzzi are you ok with the changes ? I added your suggestion 👍 thanks again for the review

@ferruzzi
Copy link
Contributor

ferruzzi commented Aug 2, 2022

Yeah, I'm good to go, but I am not a committer so you'll need to catch their attention to gt this merged in.

@potiuk ?

@potiuk potiuk merged commit 007b192 into apache:main Aug 2, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Aug 2, 2022

Awesome work, congrats on your first merged pull request!

@ferruzzi
Copy link
Contributor

ferruzzi commented Aug 2, 2022

Thanks!

This was referenced Aug 10, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ferruzzi ferruzzi ferruzzi requested changes

@potiuk potiuk potiuk approved these changes

Assignees
No one assigned
Labels
area:providers provider:amazon-aws AWS/Amazon - related issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Sagemaker Transform Job fails if there are job with Same name
4 participants
@celeriev @potiuk @ferruzzi @victorcelerier