Skip to content

SageMakerProcessingOperator does not honor action_if_job_exists #21711

New issue
New issue
Closed
#27456
Closed
SageMakerProcessingOperator does not honor action_if_job_exists#21711
#27456
Assignees
ferruzzi
Labels
area:providersgood first issuekind:bugThis is a clearly a bugprovider:amazonAWS/Amazon - related issues

Description

@iprateek
iprateek
opened on Feb 21, 2022

Apache Airflow Provider(s)

amazon

Versions of Apache Airflow Providers

apache-airflow-providers-amazon | 2.4.0

Apache Airflow version

2.2.3 (latest released)

Operating System

Amazon Linux 2

Deployment

MWAA

Deployment details

No response

What happened

Sagemaker Processing Operator no longer honors the action_if_job_exists param and always fails creation of a new processing job is a job with the name already exists.

This happens because in a recent change, the function responsible for executing the job no longer honors the increment setting:

Change that breaks the increment: 96dd703

New code:

airflow/airflow/providers/amazon/aws/operators/sagemaker.py

Line 167 in 6734eb1

def execute(self, context: 'Context') -> dict:

What you expected to happen

When Sagemaker Processing operator is called with a job-name that already exists, the job creation should succeed with a name that is incremented by 1.

How to reproduce

invoke SageMakerProcessingOperator twice with the same job name while keeping action_if_job_exists as 'increment'.

Anything else

No response

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

Activity

  1. iprateek
    added
    area:providers
    kind:bugThis is a clearly a bug
    on Feb 21, 2022

  2. boring-cyborg commented on Feb 21, 2022

    @boring-cyborg
    boring-cyborgBot
    on Feb 21, 2022

    Thanks for opening your first issue here! Be sure to follow the issue template!

  3. eladkal commented on Feb 24, 2022

    @eladkal
    eladkal
    on Feb 24, 2022
    Contributor

    Feel free to submit PR

  4. eladkal
    assigned
    iprateek
    on Feb 24, 2022
  5. eladkal
    added
    good first issue
    provider:amazonAWS/Amazon - related issues
    on Feb 24, 2022
  6. eladkal
    unassigned
    iprateek
    on Sep 30, 2022

  7. ferruzzi commented on Sep 30, 2022

    @ferruzzi
    ferruzzi
    on Sep 30, 2022
    Contributor

    It looks like this is still an issue. I am packing for a vacation, but perhaps someone else can pick it up while I'm away. The full error message is

    Traceback (most recent call last):
  File "/opt/airflow/airflow/providers/amazon/aws/operators/sagemaker.py", line 185, in execute
    raise AirflowException(
airflow.exceptions.AirflowException: A SageMaker processing job with name processing-job already exists.

    so it may be as simple as adding an if action_if_job_exists=='increment' section to the except block of this check

  8. vincbeck commented on Oct 3, 2022

    @vincbeck
    vincbeck
    on Oct 3, 2022
    Contributor

    I looked at the issue and I agree, this change 96dd703 made a breaking change by stop using the parameter action_if_job_exists. However, I dont see any valid solution to fix it. If we still want to increment the job name if action_if_job_exists is increment, then we need to use list_processing_jobs and then having back the issue #16763. The only partial valid solution I can see is to generate a random number as increment but then we might run into other issues:

    • What range should we take for this number (0..??)
    • The implementation would deviate from the function signature since we do not increment the job name but suffix the name with a random number

    My honest opinion here is we should deprecate the parameter action_if_job_exists and users should handle it themselves and apply whatever strategy they want

  9. eladkal commented on Oct 3, 2022

    @eladkal
    eladkal
    on Oct 3, 2022
    Contributor

    My honest opinion here is we should deprecate the parameter action_if_job_exists and users should handle it themselves and apply whatever strategy they want

    I'm all for it. To my prespective it goes beyond what Airflow can/should do.
    I think this is one of the cases where a blog post showing how to customize strategy is more suitable.

  10. o-nikolas commented on Oct 12, 2022

    @o-nikolas
    o-nikolas
    on Oct 12, 2022
    Contributor

    A fairly common recipe is to handle the ThrottlingException in situations like this. So the list_processing_jobs (or more specifically the _list_request helper`) can catch that exception when it's exhausted the quota, and then sleep for a second or two and then continue another burst of requests.

    This way we don't drop existing functionality and the code remains backwards compatible. We've been a bit heavy-handed with deprecations and breaking changes in the Amazon Provider package as of late.

    WDYT @vincbeck, @eladkal, @ferruzzi

  11. vincbeck commented on Oct 12, 2022

    @vincbeck
    vincbeck
    on Oct 12, 2022
    Contributor

    I am okay with that too. I still feel like this feature is odd and should have never been there but I agree, as a compromise we can go with what @o-nikolas described as solution

  12. ferruzzi
    self-assigned this
    on Oct 20, 2022

  13. ferruzzi commented on Oct 20, 2022

    @ferruzzi
    ferruzzi
    on Oct 20, 2022
    Contributor

    Sounds like a plan. I'll try to get it implemented in the next few days.

  14. potiuk
    closed this as completedin #27456on Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area:providersgood first issuekind:bugThis is a clearly a bugprovider:amazonAWS/Amazon - related issues

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions