From 23601e5a99b319a55d21e97963d0a8bade72d966 Mon Sep 17 00:00:00 2001 From: Carolyn Nguyen Date: Wed, 12 May 2021 20:41:56 -0700 Subject: [PATCH 1/5] feature: Make Choice states chainable --- CONTRIBUTING.md | 4 ++-- src/stepfunctions/steps/states.py | 14 ++++++++++++- tests/unit/test_steps.py | 34 +++++++++++++++++++++++++------ 3 files changed, 43 insertions(+), 9 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 39ba49b..705a4e3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -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 ::` @@ -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 ``` 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: diff --git a/src/stepfunctions/steps/states.py b/src/stepfunctions/steps/states.py index 7a1f093..41e79a0 100644 --- a/src/stepfunctions/steps/states.py +++ b/src/stepfunctions/steps/states.py @@ -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. + 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", + 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 diff --git a/tests/unit/test_steps.py b/tests/unit/test_steps.py index 32b118f..4307a07 100644 --- a/tests/unit/test_steps.py +++ b/tests/unit/test_steps.py @@ -12,6 +12,7 @@ # permissions and limitations under the License. from __future__ import absolute_import +import logging import pytest from stepfunctions.exceptions import DuplicateStatesInChain @@ -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') @@ -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 + + # 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) + ) + assert log_message in caplog.text + assert s2_choice.default == s1_pass + assert s2_choice.next_step is None # Choice steps do not have next_step + + def test_catch_fail_for_unsupported_state(): s1 = Pass('Step - One') From 6563f1dddfec410a3fb0199a9bfa8e46bf56ec3c Mon Sep 17 00:00:00 2001 From: Carolyn Nguyen Date: Thu, 13 May 2021 15:27:16 -0700 Subject: [PATCH 2/5] Divide test in two and extract CONTRIBUTING.md changes into another PR --- CONTRIBUTING.md | 4 ++-- src/stepfunctions/steps/states.py | 7 +++++-- tests/unit/test_steps.py | 21 ++++++++++++++------- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 705a4e3..39ba49b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -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 ::` @@ -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 main + git checkout -b my-fix-branch master ``` 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: diff --git a/src/stepfunctions/steps/states.py b/src/stepfunctions/steps/states.py index 41e79a0..6455b8a 100644 --- a/src/stepfunctions/steps/states.py +++ b/src/stepfunctions/steps/states.py @@ -221,11 +221,14 @@ def next(self, next_step): 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. + # By design, Choice states do not have the Next field. Their purpose is to define conditional transitions based + # on a set of Choice Rules. They can be viewed as an advanced Next field with multiple possible transitions. + # default_choice() sets the default transition to use in the case where none of the Choice Rules are met. + # See language spec for more info: https://states-language.net/spec.html#choice-state 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", + "Chaining Choice state: Overwriting %s's current default_choice (%s) with %s", self.state_id, self.default.state_id, next_step.state_id diff --git a/tests/unit/test_steps.py b/tests/unit/test_steps.py index 4307a07..08253fa 100644 --- a/tests/unit/test_steps.py +++ b/tests/unit/test_steps.py @@ -368,28 +368,35 @@ def test_chaining_steps(): assert s2.next_step == s3 -def test_chaining_choice(caplog): +def test_chaining_choice(): 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 + chain1 = Chain([s1_pass, s2_choice, s3_pass]) 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 + +def test_chaining_choice_with_default(caplog): + s1_pass = Pass('Step - One') + s2_choice = Choice('Step - Two') + s3_pass = Pass('Step - Three') + + s2_choice.default_choice(s3_pass) + # 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" % + expected_warning = ( + "Chaining Choice state: Overwriting %s's current default_choice (%s) with %s" % (s2_choice.state_id, s3_pass.state_id, s1_pass.state_id) ) - assert log_message in caplog.text + assert expected_warning in caplog.text + assert 'WARNING' in caplog.text assert s2_choice.default == s1_pass assert s2_choice.next_step is None # Choice steps do not have next_step From 851023fbe85d57ea3394f3fd21e99aafbb10f026 Mon Sep 17 00:00:00 2001 From: Carolyn Nguyen Date: Fri, 14 May 2021 17:54:28 -0700 Subject: [PATCH 3/5] Renamed tests and added chaining choice states info in docstings --- src/stepfunctions/steps/states.py | 7 +++---- tests/unit/test_steps.py | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/stepfunctions/steps/states.py b/src/stepfunctions/steps/states.py index 6455b8a..ea715f6 100644 --- a/src/stepfunctions/steps/states.py +++ b/src/stepfunctions/steps/states.py @@ -221,9 +221,8 @@ def next(self, next_step): 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. Their purpose is to define conditional transitions based - # on a set of Choice Rules. They can be viewed as an advanced Next field with multiple possible transitions. - # default_choice() sets the default transition to use in the case where none of the Choice Rules are met. + # By design, Choice states do not have the Next field. When used in a chain, the subsequent step becomes the + # default choice that executes if none of the specified rules match. # See language spec for more info: https://states-language.net/spec.html#choice-state if self.type is 'Choice': if self.default is not None: @@ -417,7 +416,7 @@ def allowed_fields(self): class Choice(State): """ - Choice state adds branching logic to a state machine. The state holds a list of *rule* and *next_step* pairs. The interpreter attempts pattern-matches against the rules in list order and transitions to the state or chain specified in the *next_step* field on the first *rule* where there is an exact match between the input value and a member of the comparison-operator array. + Choice state adds branching logic to a state machine. The state holds a list of *rule* and *next_step* pairs. The interpreter attempts pattern-matches against the rules in list order and transitions to the state or chain specified in the *next_step* field on the first *rule* where there is an exact match between the input value and a member of the comparison-operator array. When used in a chain, the subsequent step becomes the default choice that executes if none of the specified rules match. """ def __init__(self, state_id, **kwargs): diff --git a/tests/unit/test_steps.py b/tests/unit/test_steps.py index 08253fa..5b338bc 100644 --- a/tests/unit/test_steps.py +++ b/tests/unit/test_steps.py @@ -368,7 +368,7 @@ def test_chaining_steps(): assert s2.next_step == s3 -def test_chaining_choice(): +def test_chaining_choice_sets_default_field(): s1_pass = Pass('Step - One') s2_choice = Choice('Step - Two') s3_pass = Pass('Step - Three') @@ -381,7 +381,7 @@ def test_chaining_choice(): assert s3_pass.next_step is None -def test_chaining_choice_with_default(caplog): +def test_chaining_choice_with_existing_default_overrides_value(caplog): s1_pass = Pass('Step - One') s2_choice = Choice('Step - Two') s3_pass = Pass('Step - Three') From bce03fb96b77bc45461ef1f33ab58634a23442cd Mon Sep 17 00:00:00 2001 From: Carolyn Nguyen Date: Fri, 21 May 2021 08:17:39 -0700 Subject: [PATCH 4/5] Using fstrings --- src/stepfunctions/steps/states.py | 7 +------ tests/unit/test_steps.py | 5 +---- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/src/stepfunctions/steps/states.py b/src/stepfunctions/steps/states.py index ea715f6..9c86457 100644 --- a/src/stepfunctions/steps/states.py +++ b/src/stepfunctions/steps/states.py @@ -226,12 +226,7 @@ def next(self, next_step): # See language spec for more info: https://states-language.net/spec.html#choice-state if self.type is 'Choice': if self.default is not None: - logger.warning( - "Chaining Choice state: Overwriting %s's current default_choice (%s) with %s", - self.state_id, - self.default.state_id, - next_step.state_id - ) + logger.warning(f'Chaining Choice state: Overwriting {self.state_id}\'s current default_choice ({self.default.state_id}) with {next_step.state_id}') self.default_choice(next_step) return self.default diff --git a/tests/unit/test_steps.py b/tests/unit/test_steps.py index 5b338bc..c04bac2 100644 --- a/tests/unit/test_steps.py +++ b/tests/unit/test_steps.py @@ -391,10 +391,7 @@ def test_chaining_choice_with_existing_default_overrides_value(caplog): # Chain s2_choice when default_choice is already set will trigger Warning with caplog.at_level(logging.WARNING): Chain([s2_choice, s1_pass]) - expected_warning = ( - "Chaining Choice state: Overwriting %s's current default_choice (%s) with %s" % - (s2_choice.state_id, s3_pass.state_id, s1_pass.state_id) - ) + expected_warning = f'Chaining Choice state: Overwriting {s2_choice.state_id}\'s current default_choice ({s3_pass.state_id}) with {s1_pass.state_id}' assert expected_warning in caplog.text assert 'WARNING' in caplog.text assert s2_choice.default == s1_pass From aa82ca57ad10e1e0d34c186e9f8d309f06eb14d5 Mon Sep 17 00:00:00 2001 From: Carolyn Nguyen Date: Fri, 21 May 2021 16:50:41 -0700 Subject: [PATCH 5/5] Modified test comment --- tests/unit/test_steps.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_steps.py b/tests/unit/test_steps.py index ddc53dc..5c86279 100644 --- a/tests/unit/test_steps.py +++ b/tests/unit/test_steps.py @@ -406,7 +406,7 @@ def test_chaining_choice_with_existing_default_overrides_value(caplog): s2_choice.default_choice(s3_pass) - # Chain s2_choice when default_choice is already set will trigger Warning + # Chain s2_choice when default_choice is already set will trigger Warning message with caplog.at_level(logging.WARNING): Chain([s2_choice, s1_pass]) expected_warning = f'Chaining Choice state: Overwriting {s2_choice.state_id}\'s current default_choice ({s3_pass.state_id}) with {s1_pass.state_id}'