Make Choice states chainable #132

ca-nguyen · 2021-05-13T04:20:23Z

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 the Next field (that the Choice state does not have).

Test
Added 2 unit tests in test_steps to test:

Choice state can be chained: No error thrown and default is set.
WARNING is logged when the chained Choice state already has a default as chaining it will overwrite it

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

shivlaks

The description of changes in the commit summary should capture the rationale behind the change in addition to what is being changed.

shivlaks · 2021-05-13T05:47:39Z

tests/unit/test_steps.py

+    # 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


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 MUST transition to another state

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

Agreed, it makes sense to divide the test in 2!

shivlaks · 2021-05-13T20:54:42Z

tests/unit/test_steps.py

+                "Chaining Choice Step: Overwriting %s's current default_choice (%s) with %s" %
+                (s2_choice.state_id, s3_pass.state_id, s1_pass.state_id)


nit: might be more clear to just establish the expected string and call it expected_warning or something similar

Agreed! Made the changes in latest commit

shivlaks · 2021-05-13T20:55:08Z

tests/unit/test_steps.py

+                "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


does the log include other stuff? curious why we use in and not equality?

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

shivlaks · 2021-05-13T20:57:14Z

CONTRIBUTING.md

@@ -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


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.

Done :)
Removed the changes into another PR!

shivlaks · 2021-05-13T20:58:53Z

src/stepfunctions/steps/states.py

+        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",


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

Agreed! Done in the latest commit :)

Actually the SDK does refer to "steps" with states being a type of step.

shivlaks · 2021-05-13T21:06:27Z

src/stepfunctions/steps/states.py

            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.


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

Good point!
Done!

wong-a · 2021-05-14T22:47:18Z

tests/unit/test_steps.py

@@ -372,6 +367,40 @@ def test_chaining_steps():
    assert s1.next_step == s2
    assert s2.next_step == s3

+
+def test_chaining_choice():


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.

Suggested change

def test_chaining_choice():

def test_chaining_choice_sets_default_field():

Good point! Will make the change in the next commit.

wong-a · 2021-05-14T23:02:38Z

tests/unit/test_steps.py

+    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


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.

Agreed! Will include assertions in the generated JSON in the next commit.

As discussed offline, there is already a unit test (test_choice_state_creation) that ensures the Choice state creation generates the expected ASL

tests/unit/test_steps.py

wong-a · 2021-05-14T23:05:06Z

tests/unit/test_steps.py

+    assert s3_pass.next_step is None
+
+
+def test_chaining_choice_with_default(caplog):


Suggested change

def test_chaining_choice_with_default(caplog):

def test_chaining_choice_with_existing_default_overrides_value(caplog):

I agree, the suggested name makes the test objective clearer.

tip: you can use the Commit suggestion button to commit suggestions in the UI

wong-a · 2021-05-14T23:11:01Z

src/stepfunctions/steps/states.py

            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


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.

https://github.com/aws/aws-step-functions-data-science-sdk-python/blob/main/src/stepfunctions/steps/states.py#L402-L470

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

Very good point! Will make sure to update the public documentation as well.

yuan-bwn · 2021-05-18T02:48:25Z

src/stepfunctions/steps/states.py

+        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",


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

Good point! Using fstring will make it more readable and faster! Making changes in the next commit!

StepFunctions-Bot · 2021-05-22T01:53:05Z

AWS CodeBuild CI Report

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

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

…tion (aws/aws-step-functions-data-science-sdk-python#132)

Increments VERSION for release to PyPI which will include the following changes since v2.0.0: * feature: Make Choice states chainable #132 * fix: Make service integration ARNs partition aware #131

ca-nguyen added 2 commits May 12, 2021 20:41

feature: Make Choice states chainable

23601e5

Merge branch 'main' into chain-choice-states

eb6fc49

ca-nguyen requested review from wong-a, shivlaks and yuan-bwn May 13, 2021 04:20

shivlaks suggested changes May 13, 2021

View reviewed changes

Divide test in two and extract CONTRIBUTING.md changes into another PR

6563f1d

wong-a suggested changes May 14, 2021

View reviewed changes

ca-nguyen requested a review from shivlaks May 14, 2021 23:44

Renamed tests and added chaining choice states info in docstings

851023f

ca-nguyen requested a review from wong-a May 15, 2021 00:54

yuan-bwn reviewed May 18, 2021

View reviewed changes

wong-a previously approved these changes May 19, 2021

View reviewed changes

shivlaks previously approved these changes May 21, 2021

View reviewed changes

Merge branch 'main' into chain-choice-states

2c72de7

ca-nguyen dismissed stale reviews from shivlaks and wong-a via 2c72de7 May 21, 2021 15:00

shivlaks previously approved these changes May 21, 2021

View reviewed changes

ca-nguyen added 3 commits May 21, 2021 08:17

Using fstrings

bce03fb

Using fstrings and merged with latest changes on remote

c4811f3

Merging

44882aa

ca-nguyen dismissed shivlaks’s stale review via 44882aa May 21, 2021 15:30

ca-nguyen requested review from yuan-bwn, wong-a and shivlaks May 21, 2021 15:31

wong-a previously approved these changes May 21, 2021

View reviewed changes

Modified test comment

aa82ca5

ca-nguyen dismissed wong-a’s stale review via aa82ca5 May 21, 2021 23:50

wong-a approved these changes May 25, 2021

View reviewed changes

shivlaks approved these changes May 25, 2021

View reviewed changes

ca-nguyen merged commit 6091932 into aws:main May 25, 2021

DyfanJones pushed a commit to DyfanJones/aws-step-functions-data-science-sdk-r that referenced this pull request May 26, 2021

feature: enable choice to be chainable, to align with python stepfunc…

f1d5538

…tion (aws/aws-step-functions-data-science-sdk-python#132)

DyfanJones mentioned this pull request May 26, 2021

feature: enable choice to be chainable, to align with python stepfunc… DyfanJones/aws-step-functions-data-science-sdk-r#18

Merged

wong-a mentioned this pull request May 27, 2021

Bump version to v2.1.0 #135

Merged

wong-a added a commit that referenced this pull request May 27, 2021

v2.1.0 (#135)

c82bd52

Increments VERSION for release to PyPI which will include the following changes since v2.0.0: * feature: Make Choice states chainable #132 * fix: Make service integration ARNs partition aware #131

ca-nguyen deleted the chain-choice-states branch October 27, 2021 01:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Choice states chainable #132

Make Choice states chainable #132

ca-nguyen commented May 13, 2021 •

edited

Loading

shivlaks left a comment

shivlaks May 13, 2021

ca-nguyen May 13, 2021

shivlaks May 13, 2021

ca-nguyen May 14, 2021

shivlaks May 13, 2021

ca-nguyen May 13, 2021

shivlaks May 13, 2021

ca-nguyen May 14, 2021

shivlaks May 13, 2021

ca-nguyen May 14, 2021

wong-a May 15, 2021

shivlaks May 13, 2021

ca-nguyen May 14, 2021

wong-a May 14, 2021

ca-nguyen May 14, 2021

wong-a May 14, 2021

ca-nguyen May 14, 2021

ca-nguyen May 15, 2021

wong-a May 14, 2021

ca-nguyen May 14, 2021

wong-a May 19, 2021

wong-a May 14, 2021

ca-nguyen May 15, 2021

yuan-bwn May 18, 2021 •

edited

Loading

ca-nguyen May 21, 2021

StepFunctions-Bot commented May 22, 2021

		"Chaining Choice Step: Overwriting %s's current default_choice (%s) with %s" %
		(s2_choice.state_id, s3_pass.state_id, s1_pass.state_id)

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

		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.

	def test_chaining_choice():
	def test_chaining_choice_sets_default_field():

		assert s3_pass.next_step is None


		def test_chaining_choice_with_default(caplog):

	def test_chaining_choice_with_default(caplog):
	def test_chaining_choice_with_existing_default_overrides_value(caplog):

Make Choice states chainable #132

Make Choice states chainable #132

Conversation

ca-nguyen commented May 13, 2021 • edited Loading

shivlaks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuan-bwn May 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StepFunctions-Bot commented May 22, 2021

AWS CodeBuild CI Report

ca-nguyen commented May 13, 2021 •

edited

Loading

yuan-bwn May 18, 2021 •

edited

Loading