Skip to content

StepFunctions: Fix Evaluation of Nested Map Runs #12033

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 13, 2024
Merged

StepFunctions: Fix Evaluation of Nested Map Runs #12033

merged 1 commit into from
Dec 13, 2024

Conversation

MEPalma
Copy link
Contributor

@MEPalma MEPalma commented Dec 13, 2024

Motivation

Currently, the SFN v2 is unable to evaluate nested MapRun states, resulting in pickling runtime errors #9809. This was due to an unecessary deep copy of the map run program, which contains unserialisable objects such as locks. The deep copy is not considered necessary as EvalComponents are stateless by design, and can therefore be shared by multiple runtimes. These changes address such issue and add a relevant snapshot test.

Changes

  • remove unecessary program copy
  • added a snapshot test for nested map runs

@MEPalma MEPalma added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Dec 13, 2024
@MEPalma MEPalma added this to the 4.1 milestone Dec 13, 2024
@MEPalma MEPalma self-assigned this Dec 13, 2024
gregfurman
gregfurman approved these changes Dec 13, 2024
View reviewed changes
Copy link
Contributor

@gregfurman gregfurman left a comment

Choose a reason for hiding this comment

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

LGTM!

@github-actions GitHub Actions
Copy link

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   35m 44s ⏱️ - 1h 17m 3s
1 333 tests  - 2 560  1 269 ✅  - 2 314  64 💤  - 246  0 ❌ ±0 
1 335 runs   - 2 560  1 269 ✅  - 2 314  66 💤  - 246  0 ❌ ±0 

Results for commit a55ec15. ± Comparison against base commit 9f28c06.

This pull request removes 2561 and adds 1 tests. Note that renamed tests count towards both. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

tests.aws.services.stepfunctions.v2.scenarios.test_base_scenarios.TestBaseScenarios ‑ test_map_state_nested_config_distributed

@MEPalma MEPalma merged commit cb66b15 into master Dec 13, 2024
36 checks passed
@MEPalma MEPalma deleted the MEP-sfn-fix_pickle_nested_map_run branch December 13, 2024 15:04
@MEPalma MEPalma mentioned this pull request Dec 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregfurman gregfurman gregfurman approved these changes

@joe4dev joe4dev Awaiting requested review from joe4dev

@dominikschubert dominikschubert Awaiting requested review from dominikschubert

Assignees

@MEPalma MEPalma

Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.
2 participants
@MEPalma @gregfurman