Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 25, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ Before sending us a pull request, please ensure that:
### Running the Unit Tests

1. Install tox using `pip install tox`
1. Install test dependencies, including coverage, using `pip install .[test]`
1. cd into the aws-step-functions-data-science-sdk-python folder: `cd aws-step-functions-data-science-sdk-python` or `cd /environment/aws-step-functions-data-science-sdk-python`
1. Install test dependencies, including coverage, using `pip install ".[test]"`
1. Run the following tox command and verify that all code checks and unit tests pass: `tox tests/unit`

You can also run a single test with the following command: `tox -e py36 -- -s -vv <path_to_file><file_name>::<test_function_name>`
Expand All @@ -80,7 +80,7 @@ You should only worry about manually running any new integration tests that you

1. Create a new git branch:
```shell
git checkout -b my-fix-branch master
git checkout -b my-fix-branch main
Copy link
Contributor

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.

Copy link
Contributor Author

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 another PR!

```
1. Make your changes, **including unit tests** and, if appropriate, integration tests.
1. Include unit tests when you contribute new features or make bug fixes, as they help to:
Expand Down
14 changes: 13 additions & 1 deletion src/stepfunctions/steps/states.py
Original file line number Diff line number Diff line change
Expand Up @@ -218,9 +218,21 @@ def next(self, next_step):
Returns:
State or Chain: Next state or chain that will be transitioned to.
"""
if self.type in ('Choice', 'Succeed', 'Fail'):
if self.type in ('Succeed', 'Fail'):
raise ValueError('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.
Copy link
Contributor

Choose a reason for hiding this comment

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

explain the why rather than the what so that future contributors can gain insight.

there's some great info in the gist that @wong-a created that could be paraphrased. Might also be helpful to link to the language spec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!
Done!

if self.type is 'Choice':
if self.default is not None:
logger.warning(
"Chaining Choice Step: Overwriting %s's current default_choice (%s) with %s",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Chaining Choice Step: Overwriting %s's current default_choice (%s) with %s",
"Chaining Choice state: Overwriting %s's current default_choice (%s) with %s",

I think we should be consistent with calling it a state

Copy link
Contributor Author

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 :)

Copy link
Contributor

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.

self.state_id,
self.default.state_id,
next_step.state_id
)
self.default_choice(next_step)
return self.default

self.next_step = next_step
return self.next_step

Expand Down
34 changes: 28 additions & 6 deletions tests/unit/test_steps.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
# permissions and limitations under the License.
from __future__ import absolute_import

import logging
import pytest

from stepfunctions.exceptions import DuplicateStatesInChain
Expand Down Expand Up @@ -328,12 +329,6 @@ def test_append_states_after_terminal_state_will_fail():
chain.append(Succeed('Succeed'))
chain.append(Pass('Pass2'))

with pytest.raises(ValueError):
chain = Chain()
chain.append(Pass('Pass'))
chain.append(Choice('Choice'))
chain.append(Pass('Pass2'))


def test_chaining_steps():
s1 = Pass('Step - One')
Expand Down Expand Up @@ -372,6 +367,33 @@ def test_chaining_steps():
assert s1.next_step == s2
assert s2.next_step == s3


def test_chaining_choice(caplog):
s1_pass = Pass('Step - One')
s2_choice = Choice('Step - Two')
s3_pass = Pass('Step - Three')

with caplog.at_level(logging.WARNING):
chain1 = Chain([s1_pass, s2_choice, s3_pass])
assert caplog.text == '' # No warning
assert chain1.steps == [s1_pass, s2_choice, s3_pass]
assert s1_pass.next_step == s2_choice
assert s2_choice.default == s3_pass
assert s2_choice.next_step is None # Choice steps do not have next_step
assert s3_pass.next_step is None
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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


# Chain s2_choice when default_choice is already set will trigger Warning
with caplog.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)
Copy link
Contributor

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 it expected_warning or something similar

Copy link
Contributor Author

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

)
assert log_message in caplog.text
Copy link
Contributor

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 use in and not equality?

Copy link
Contributor Author

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

assert s2_choice.default == s1_pass
assert s2_choice.next_step is None # Choice steps do not have next_step
Copy link
Contributor

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:

  1. test that choice state has a default that is established if there wasn't one because it MUST transition to another state
  2. test that warning is produced when a default state is already set and a choice state is chained because the behaviour is counter-intuitive + overwrites an existing value

Copy link
Contributor Author

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!



def test_catch_fail_for_unsupported_state():
s1 = Pass('Step - One')

Expand Down