aws/aws-step-functions-data-science-sdk-pythonPublic

NotificationsYou must be signed in to change notification settings
Fork89
Star292

fix: Retrier and Catcher passed to constructor for Task, Parallel and Map states are not added to the state's Retriers and Catchers#169

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

ca-nguyen merged 7 commits intoaws:mainfromca-nguyen:fix-retry-in-task-constructor

Oct 7, 2021

Merged

fix: Retrier and Catcher passed to constructor for Task, Parallel and Map states are not added to the state's Retriers and Catchers#169

ca-nguyen merged 7 commits intoaws:mainfromca-nguyen:fix-retry-in-task-constructor

Oct 7, 2021

Conversation

Copy link

Contributor

ca-nguyen commentedSep 23, 2021•
edited
Loading

Description

This will allow retry and catch blocks to be added directly from the constructor.

Fixes#115

Why is the change necessary?

Currently,retry andcatch blocks 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 (seeTask ASL doc for ex). This PR makes it possible to use lists or Retry/Catch objects in the constructor forretry andcatch constructor arguments.

Solution

In the constructor, whenretry/catch is 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 andMap.

Testing

Added unit and integ tests
Manual testing

Action Items

TODO in a separate PR:

Update CONTRIBUTING guide in the testing section to use descriptive method names following thetest_<unit_under_test>_<state_or_input_under_test>_<expectation> naming convention

Pull Request Checklist

Please check all boxes (including N/A items)

Testing

Unit tests added
integration test added

Documentation

docs: All relevantdocs updated
docstrings: All public APIs documented

Title and description

Change type: Title is prefixed with change type: and followsconventional commits
References: Indicate issues fixed via:Fixes #xxx

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

ca-nguyen added3 commits

September 22, 2021 17:21

Add retry to Retriers when passed to constructor and add catch to Cat…

79c66c3

…chers when passed to constructor

Added integ tests

7f0ceff

Use fstring in raised exception

30e8da9

shivlaks suggested changes

Oct 5, 2021

View reviewed changes

Copy link

Contributor

shivlaks left a comment

Choose a reason for hiding this comment

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

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 theTask,Parallel, andMap states. 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.

src/stepfunctions/steps/states.pyShow resolvedHide resolved

tests/unit/test_steps.py OutdatedShow resolvedHide resolved

tests/integ/test_state_machine_definition.py OutdatedShow resolvedHide resolved

wong-a suggested changes

Oct 6, 2021

View reviewed changes

tests/unit/test_steps.py OutdatedShow resolvedHide resolved

src/stepfunctions/steps/states.py OutdatedShow resolvedHide resolved

ca-nguyen changed the title~~feat: Add retry and catch to Retriers and Catchers when passed to constructor~~fix: Retrier and Catcher passed to constructor for Task, Parallel and Map states are not added to the state's Retriers and Catchers

Oct 7, 2021

ca-nguyenand others added3 commits

October 6, 2021 22:02

Merge branch 'main' into fix-retry-in-task-constructor

0582e91

Write unit test for each state and update docstrings to use ASL retri…

abf2656

…er and catcher terms

Merge branch 'fix-retry-in-task-constructor' ofhttps://github.com/ca…

f3a0cc1

…-nguyen/aws-step-functions-data-science-sdk-python into fix-retry-in-task-constructor

wong-a previously approved these changes

Oct 7, 2021

View reviewed changes

Copy link

Contributor

wong-a left a comment

Choose a reason for hiding this comment

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

LGTM besides Shiv's comment on the integ test.

tests/unit/test_steps.py

		(CATCH,EXPECTED_CATCH),
		(CATCHES,EXPECTED_CATCHES)
		])
		deftest_parallel_state_constructor_with_catch_adds_catcher_to_catchers(catch,expected_catch):

Copy link

Contributor

wong-aOct 7, 2021

Choose a reason for hiding this comment

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

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.

Copy link

Contributor

shivlaksOct 7, 2021

Choose a reason for hiding this comment

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

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

tests/integ/test_state_machine_definition.pyShow resolvedHide resolved

tests/integ/test_state_machine_definition.py OutdatedShow resolvedHide resolved

Removed integ test for retry and catch in constructor and updated exi…

cd1fc9e

…sting tests

ca-nguyen dismissedwong-a’sstale review viacd1fc9e

October 7, 2021 19:36

ca-nguyen requested review fromshivlaks andwong-a

October 7, 2021 19:40

wong-a approved these changes

Oct 7, 2021

View reviewed changes

Copy link

Contributor

wong-a left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link

Contributor

StepFunctions-Bot commentedOct 7, 2021

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-sEHrOdk7acJc
Commit ID:cd1fc9e
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered bygithub-codebuild-logs, available on theAWS Serverless Application Repository

shivlaks approved these changes

Oct 7, 2021

View reviewed changes

tests/unit/test_steps.py

		(CATCH,EXPECTED_CATCH),
		(CATCHES,EXPECTED_CATCHES)
		])
		deftest_parallel_state_constructor_with_catch_adds_catcher_to_catchers(catch,expected_catch):

Copy link

Contributor

shivlaksOct 7, 2021

Choose a reason for hiding this comment

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

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

tests/integ/test_state_machine_definition.py

		task_resource=f"arn:{get_aws_partition()}:states:::sagemaker:createTrainingJob.sync"

		#change the parameters to causetask state to fail
		#Provide invalid TrainingImage to causeStates.TaskFailed error

Copy link

Contributor

shivlaksOct 7, 2021

Choose a reason for hiding this comment

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

thank you, it's a lot less mysterious what we're trying to do now

ca-nguyen merged commit7f223e8 intoaws:main

Oct 7, 2021

ca-nguyen mentioned this pull request

Oct 7, 2021

Can't serialize graphs using Retry#115

Closed

ca-nguyen deleted the fix-retry-in-task-constructor branch

October 27, 2021 01:12

ca-nguyen mentioned this pull request

Dec 4, 2021

chore: Bump version to v2.3.0#183

Merged

Labels

None yet

Movatterモバイル変換

fix: Retrier and Catcher passed to constructor for Task, Parallel and Map states are not added to the state's Retriers and Catchers#169

fix: Retrier and Catcher passed to constructor for Task, Parallel and Map states are not added to the state's Retriers and Catchers#169

Uh oh!

Conversation

ca-nguyen commentedSep 23, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Description

Why is the change necessary?

Solution

Testing

Action Items

Pull Request Checklist

Testing

Documentation

Title and description

Uh oh!

shivlaks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wong-a left a comment

Choose a reason for hiding this comment

Uh oh!

wong-aOct 7, 2021

Choose a reason for hiding this comment

Uh oh!

shivlaksOct 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wong-a left a comment

Choose a reason for hiding this comment

Uh oh!

StepFunctions-Bot commentedOct 7, 2021

AWS CodeBuild CI Report

Uh oh!

shivlaksOct 7, 2021

Choose a reason for hiding this comment

Uh oh!

shivlaksOct 7, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ca-nguyen commentedSep 23, 2021•
edited
Loading