- Notifications
You must be signed in to change notification settings - Fork89
Make Choice states chainable#132
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
shivlaks left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The description of changes in the commit summary should capture the rationale behind the change in addition to what is being changed.
tests/unit/test_steps.py Outdated
| # Chain s2_choice when default_choice is already set will trigger Warning | ||
| withcaplog.at_level(logging.WARNING): | ||
| Chain([s2_choice,s1_pass]) | ||
| log_message= ( | ||
| "Chaining Choice Step: Overwriting %s's current default_choice (%s) with %s"% | ||
| (s2_choice.state_id,s3_pass.state_id,s1_pass.state_id) | ||
| ) | ||
| assertlog_messageincaplog.text | ||
| asserts2_choice.default==s1_pass | ||
| asserts2_choice.next_stepisNone# Choice steps do not have next_step |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
i'm not sure whether this is an idiomatic Python thing, but I'd extract this into a separate test:
the way I interpret the control flow, there are 2 scenarios and sets of assertions.
The 2 tests from what i can tell:
- test that choice state has a default that is established if there wasn't one because it
MUSTtransition to another state - test that warning is produced when a default state is already setand a choice state is chained because the behaviour is counter-intuitive + overwrites an existing value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agreed, it makes sense to divide the test in 2!
tests/unit/test_steps.py Outdated
| "Chaining Choice Step: Overwriting %s's current default_choice (%s) with %s"% | ||
| (s2_choice.state_id,s3_pass.state_id,s1_pass.state_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: might be more clear to just establish the expected string and call itexpected_warning or something similar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agreed! Made the changes in latest commit
tests/unit/test_steps.py Outdated
| "Chaining Choice Step: Overwriting %s's current default_choice (%s) with %s"% | ||
| (s2_choice.state_id,s3_pass.state_id,s1_pass.state_id) | ||
| ) | ||
| assertlog_messageincaplog.text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
does the log include other stuff? curious why we usein and not equality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, it includes the logging type (in our case WARNING) and the line(and file) where it occurred.
I can add an assert to check if "WARNING" is in caplog.text
CONTRIBUTING.md Outdated
| 1. Create a new git branch: | ||
| ```shell | ||
| git checkout -b my-fix-branchmaster | ||
| git checkout -b my-fix-branchmain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: prefer these in a separate PR to observe our guidance to focus on the change being introduced. this PR is small so it doesn't take away focus, but just something to keep in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done :)
Removed the changes into anotherPR!
src/stepfunctions/steps/states.py Outdated
| ifself.typeis'Choice': | ||
| ifself.defaultisnotNone: | ||
| logger.warning( | ||
| "Chaining Choice Step: Overwriting %s's current default_choice (%s) with %s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| "Chaining ChoiceStep: Overwriting %s's current default_choice (%s) with %s", | |
| "Chaining Choicestate: Overwriting %s's current default_choice (%s) with %s", |
I think we should be consistent with calling it astate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agreed! Done in the latest commit :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually the SDK does refer to "steps" with states being a type of step.
src/stepfunctions/steps/states.py Outdated
| ifself.typein ('Succeed','Fail'): | ||
| raiseValueError('Unexpected State instance `{step}`, State type `{state_type}` does not support method `next`.'.format(step=next_step,state_type=self.type)) | ||
| # By design, choice states do not have the Next field. Setting default to make it chainable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
explain thewhy rather than thewhat so that future contributors can gain insight.
there's some great info in thegist that@wong-a created that could be paraphrased. Might also be helpful to link to thelanguage spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good point!
Done!
tests/unit/test_steps.py Outdated
| asserts2.next_step==s3 | ||
| deftest_chaining_choice(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's helpful to include the unit being tested, the inputs, and the expected outcome in the test names. This acts documentation for the expected behaviour.
| deftest_chaining_choice(): | |
| deftest_chaining_choice_sets_default_field(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good point! Will make the change in the next commit.
| asserts1_pass.next_step==s2_choice | ||
| asserts2_choice.default==s3_pass | ||
| asserts2_choice.next_stepisNone# Choice steps do not have next_step | ||
| asserts3_pass.next_stepisNone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
When writing unit tests for these modules we should consider making assertions on the generated JSON too. One of the main responsibilities is generating ASL so we should make sure it's correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agreed! Will include assertions in the generated JSON in the next commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As discussed offline, there is already a unit test (test_choice_state_creation) that ensures the Choice state creation generates the expected ASL
Uh oh!
There was an error while loading.Please reload this page.
tests/unit/test_steps.py Outdated
| asserts3_pass.next_stepisNone | ||
| deftest_chaining_choice_with_default(caplog): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| deftest_chaining_choice_with_default(caplog): | |
| deftest_chaining_choice_with_existing_default_overrides_value(caplog): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree, the suggested name makes the test objective clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
tip: you can use theCommit suggestion button to commit suggestions in the UI
src/stepfunctions/steps/states.py Outdated
| ifself.typein ('Succeed','Fail'): | ||
| raiseValueError('Unexpected State instance `{step}`, State type `{state_type}` does not support method `next`.'.format(step=next_step,state_type=self.type)) | ||
| # By design, Choice states do not have the Next field. Their purpose is to define conditional transitions based |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should document the behaviour of chaining Choice states publicly. Consumers of the SDK won't be looking at the source code. Something along the lines of:
When used in a chain, the subsequent step becomes the default choice that executes if none of the specified rules match.
The docstrings in our source code become part of the public documentation and are visible in user's IDE:https://aws-step-functions-data-science-sdk.readthedocs.io/en/stable/states.html#stepfunctions.steps.states.Choice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Very good point! Will make sure to update the public documentation as well.
src/stepfunctions/steps/states.py Outdated
| ifself.typeis'Choice': | ||
| ifself.defaultisnotNone: | ||
| logger.warning( | ||
| "Chaining Choice state: Overwriting %s's current default_choice (%s) with %s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: not a blocker but can try using f-string next time:https://www.python.org/dev/peps/pep-0498/
Got this recommendation from Shiv and think this could also help here. Same for the unit test string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good point! Using fstring will make it more readable and faster! Making changes in the next commit!
StepFunctions-Bot commentedMay 22, 2021
AWS CodeBuild CI Report
Powered bygithub-codebuild-logs, available on theAWS Serverless Application Repository |
Uh oh!
There was an error while loading.Please reload this page.
Issue #, if available:31
Description of changes:
It was discussed in issue#31 that the non terminal Choice state behaviour was not obvious since it could not be chained. This PR allows Choice states to be chained by setting default_choice() instead of theNext field (that the Choice state does not have).
Test
Added 2 unit tests in test_steps to test:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.