fix: Retrier and Catcher passed to constructor for Task, Parallel and Map states are not added to the state's Retriers and Catchers #169
Conversation
…chers when passed to constructor
There was a problem hiding this comment.
great job!
couple of small general things
- this is a fix... the discarding of user provided parameters in constructor is a bug that we are addressing rather than a feature.
- let's tighten up the description to also include that this is for the
Task,Parallel, andMapstates. This is covered in the subsequent section but I think it belongs in the title - "Manual Testing" - this should detail what manual testing we performed. Another contributor should be able to read your description and perform the same manual test. It's both a teaching tool as well as a valuable artifact for posterity.
ca-nguyen
changed the title
…er and catcher terms
…-nguyen/aws-step-functions-data-science-sdk-python into fix-retry-in-task-constructor
| (CATCH, EXPECTED_CATCH), | ||
| (CATCHES, EXPECTED_CATCHES) | ||
| ]) | ||
| def test_parallel_state_constructor_with_catch_adds_catcher_to_catchers(catch, expected_catch): |
There was a problem hiding this comment.
comment (non-blocking): This reads a lot better than before. I still don't think each test case needs to be parameterized at all, but that's nit picking/preference at this point.
There was a problem hiding this comment.
agreed. im of the same opinion - it's a lot easier to understand what a test is doing.
verbosity in tests like these does more good than harm
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| (CATCH, EXPECTED_CATCH), | ||
| (CATCHES, EXPECTED_CATCHES) | ||
| ]) | ||
| def test_parallel_state_constructor_with_catch_adds_catcher_to_catchers(catch, expected_catch): |
There was a problem hiding this comment.
agreed. im of the same opinion - it's a lot easier to understand what a test is doing.
verbosity in tests like these does more good than harm
| interval_seconds = 1 | ||
| max_attempts = 2 | ||
| backoff_rate = 2 | ||
| task_resource = f"arn:{get_aws_partition()}:states:::sagemaker:createTrainingJob.sync" | ||
|
|
||
| # change the parameters to cause task state to fail | ||
| # Provide invalid TrainingImage to cause States.TaskFailed error |
There was a problem hiding this comment.
thank you, it's a lot less mysterious what we're trying to do now
Description
This will allow retry and catch blocks to be added directly from the constructor.
Fixes #115
Why is the change necessary?
Currently,
retryandcatchblocks passed to Task, Parallel and Map State constructors are not added to those state's Retriers and Catchers. With this change, it will possible do so.Per the ASL documentation, Retry and Catch fields are arrays that contain Retriers and Catchers (see Task ASL doc for ex). This PR makes it possible to use lists or Retry/Catch objects in the constructor for
retryandcatchconstructor arguments.Solution
In the constructor, when
retry/catchis notNone, add it to the state's retries/catches.This change is done for the 3 States that support Retry and Catch blocks: Task, Parallel and Map.
Testing
Action Items
TODO in a separate PR:
test_<unit_under_test>_<state_or_input_under_test>_<expectation>naming conventionPull Request Checklist
Please check all boxes (including N/A items)
Testing
Documentation
Title and description
Fixes #xxxBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.