Core: Add type hints to aws/core.py #12617

bblommers · 2025-05-14T10:05:43Z

Motivation

The main reason for wanting types in this file is to make sure that context.account_id and context.region are always a string.

The handler chain will always set defaults for these variables, so we know they are set at runtime. Explicitly marking them as str makes it more obvious for service developers that they can trust the variables are set, and they don't have to manually assert that they are not None.

Changes

Most of the changes are self-explanatory, but the more spicier ones:

Installing `botocore` and `rolo` during linting

CircleCI now installs all typehint-dependencies during installation.

In our local venv we should have requirements-typehints installed, which means we have all the types - but the precommit-hook runs in an custom venv without those dependencies, so it will complain that they are missing. There are workarounds, for instance by creating a custom hook for mypy, but explicitly installing the dependencies we need during the pre-commit step is the easiest solution for now.

RequestContext API change

The request-parameter in RoloRequestContext is mandatory, and we do always supply it - so I've changed it to a mandatory parameter, and changed all the places where we create a RequestContext to add the request as an __init__-parameter.

Note that this class is never explicitly created in the AWS Pro repo, so we don't have to worry much about breaking changes. It is instantiated in Another emulator, but breaking changes are less important there, so I'll change that afterwards.

github-actions · 2025-05-14T10:14:45Z

S3 Image Test Results (AMD64 / ARM64)

2 files 2 suites 9m 15s ⏱️
495 tests 445 ✅ 50 💤 0 ❌
990 runs 890 ✅ 100 💤 0 ❌

Results for commit 412626b.

♻️ This comment has been updated with latest results.

github-actions · 2025-05-14T12:56:48Z

LocalStack Community integration with Pro

2 files ±0 2 suites ±0 1h 46m 2s ⏱️ +48s
4 452 tests ±0 4 067 ✅ ±0 385 💤 ±0 0 ❌ ±0
4 454 runs ±0 4 067 ✅ ±0 387 💤 ±0 0 ❌ ±0

Results for commit 412626b. ± Comparison against base commit fa0bbcd.

♻️ This comment has been updated with latest results.

alexrashed

Awesome! This is another great push for MyPy type enforcements!
From my perspective the changes to the typing are great, I would just love to have @k-a-il or @silv-io approve the change / verify that this would not break the GitHub workflow which is currently being implemented :)

alexrashed · 2025-05-14T13:46:01Z

.circleci/config.yml

      - run:
          name: Setup environment
          command: |
+            make install-dev-types


@k-a-il @silv-io Now that the CircleCI pipeline is being migrated to GitHub Actions I think this would mean that we also have to change the make target here, right?

localstack/.github/actions/setup-tests-env/action.yml

Line 15 in 9b6ef35

run: make install-dev

Yes, would be great to add this in this PR.

On the other hand I think we should also be cautious with this switch because the typehint dependencies take waaaay longer to install in my experience (except when using uv :D)

Well, is this a blocker then? What does "waaaay longer" mean here?

@silv-io Could you please give some guidance here?

What does it mean to update the other action?

Where do we need to adjust the dependency installation for the type checking?

How can we properly verify that this PR covers this aspect, since it's already fully green?

What are the timing implications of this change you mentioned and what is acceptable for this PR to be merged?

Yes :)

I'll add a suggestion in my review

After doing the suggested changes, the AWS / Build, Test, Push pipeline will run on this PR and we'll see if it works there as well. (We've still got some unrelated test failures around the Docker SDK there. These will not be blocking this PR)

I've checked the CircleCI Pipeline and it looks like it will add 30s of installation time. Seems like caching works a bit better here than on-device, so the impact is not as big. The trade-off is fine in this case.

silv-io

The changes in CircleCI look fine to me. The increase in installation time is not too big and completely justified by the extra functionality we're gaining.

Before merging this, I'd love to have these features in the GitHub Actions pipeline as well. To do this, we'd need to change these two actions to install the typehint dependency set instead of only the dev dependencies:

localstack/.github/actions/setup-tests-env/action.yml

Lines 6 to 15 in 940aa0a

    
           - name: Set up Python 
        
             uses: actions/setup-python@v5 
        
             with: 
        
               python-version-file: '.python-version' 
        
               cache: 'pip' 
        
               cache-dependency-path: 'requirements-dev.txt' 
        
           - name: Install Community Dependencies 
        
             shell: bash 
        
             run: make install-dev

localstack/.github/actions/load-localstack-docker-from-artifacts/action.yml

Lines 16 to 21 in 549fd9b

    
           - name: Set up Python 
        
             uses: actions/setup-python@v5 
        
             with: 
        
               python-version-file: '.python-version' 
        
               cache: 'pip' 
        
               cache-dependency-path: 'requirements-dev.txt'

Changing these should also trigger the new GHA pipeline. We can ignore single integration test failures on that pipeline since we've still got some issues there. The other steps, especially the linting, should still be green though.

/cc @k-a-il

silv-io · 2025-05-15T08:25:23Z

.circleci/config.yml

      - run:
          name: Setup environment
          command: |
+            make install-dev-types


Yes :)

I'll add a suggestion in my review

After doing the suggested changes, the AWS / Build, Test, Push pipeline will run on this PR and we'll see if it works there as well. (We've still got some unrelated test failures around the Docker SDK there. These will not be blocking this PR)

I've checked the CircleCI Pipeline and it looks like it will add 30s of installation time. Seems like caching works a bit better here than on-device, so the impact is not as big. The trade-off is fine in this case.

bblommers · 2025-05-19T09:26:29Z

@silv-io Thanks for checking! I've also changed the GH pipelines to now install requirements-typehints.

I've also changed the pipeline to use the load-localstack-docker-from-artifacts/setup-tests-env actions from the current branch, instead of from master - otherwise we'd never be able to test changes to those actions.

k-a-il

Nice, thank you for fixing paths to actions 👍 LGTM

k-a-il · 2025-05-19T10:43:48Z

I just merged my fix for the integration tests TestDockerClient and TestHooks. If you rebase your branch, the tests AWS / MA/MR tests / Run integration tests will pass.

silv-io

LGTM! Thank you for the changes

bblommers · 2025-05-20T09:44:08Z

Appreciate the review and feedback @silv-io and @k-a-il 🙏

bblommers added this to the Playground milestone May 14, 2025

bblommers requested review from alexrashed and silv-io May 14, 2025 10:05

bblommers added the semver: patch Non-breaking changes which can be included in patch releases label May 14, 2025

bblommers force-pushed the typing-aws-core branch 5 times, most recently from 3213dac to b0e5536 Compare May 14, 2025 11:54

bblommers marked this pull request as ready for review May 14, 2025 12:57

bblommers requested review from bentsku, sannya-singal, thrau, dfangl, dominikschubert and k-a-il as code owners May 14, 2025 12:57

alexrashed reviewed May 14, 2025

View reviewed changes

silv-io requested changes May 15, 2025

View reviewed changes

bblommers force-pushed the typing-aws-core branch from b0e5536 to e4a6c59 Compare May 15, 2025 09:15

bblommers marked this pull request as draft May 15, 2025 09:16

bblommers force-pushed the typing-aws-core branch 5 times, most recently from ce0a7e8 to bb82b54 Compare May 19, 2025 08:11

bblommers marked this pull request as ready for review May 19, 2025 09:26

bblommers requested a review from silv-io May 19, 2025 09:26

k-a-il approved these changes May 19, 2025

View reviewed changes

Core: Add type hints to aws/core.py

412626b

bblommers force-pushed the typing-aws-core branch from bb82b54 to 412626b Compare May 19, 2025 17:06

silv-io approved these changes May 20, 2025

View reviewed changes

bblommers merged commit 70b8643 into master May 20, 2025
82 of 84 checks passed

bblommers deleted the typing-aws-core branch May 20, 2025 09:44

bblommers mentioned this pull request May 20, 2025

Transcribe: Enable MyPy #12588

Open

	- name: Set up Python
	uses: actions/setup-python@v5
	with:
	python-version-file: '.python-version'
	cache: 'pip'
	cache-dependency-path: 'requirements-dev.txt'

	- name: Install Community Dependencies
	shell: bash
	run: make install-dev

Uh oh!

Core: Add type hints to aws/core.py #12617

Core: Add type hints to aws/core.py #12617

Uh oh!

Conversation

bblommers commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Installing botocore and rolo during linting

RequestContext API change

Uh oh!

github-actions bot commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

S3 Image Test Results (AMD64 / ARM64)

Uh oh!

github-actions bot commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LocalStack Community integration with Pro

Uh oh!

alexrashed left a comment

Choose a reason for hiding this comment

Uh oh!

alexrashed May 14, 2025

Choose a reason for hiding this comment

Uh oh!

silv-io May 14, 2025

Choose a reason for hiding this comment

Uh oh!

alexrashed May 14, 2025

Choose a reason for hiding this comment

Uh oh!

alexrashed May 15, 2025

Choose a reason for hiding this comment

Uh oh!

silv-io May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silv-io left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silv-io May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bblommers commented May 19, 2025

Uh oh!

k-a-il left a comment

Choose a reason for hiding this comment

Uh oh!

k-a-il commented May 19, 2025

Uh oh!

silv-io left a comment

Choose a reason for hiding this comment

Uh oh!

bblommers commented May 20, 2025

Uh oh!

Uh oh!

Uh oh!

bblommers commented May 14, 2025 •

edited

Loading

Installing `botocore` and `rolo` during linting

github-actions bot commented May 14, 2025 •

edited

Loading

github-actions bot commented May 14, 2025 •

edited

Loading

silv-io May 15, 2025 •

edited

Loading

silv-io left a comment •

edited

Loading

silv-io May 15, 2025 •

edited

Loading