Updates repository to use UV and Python 3.14#7
Conversation
ghukill
left a comment
There was a problem hiding this comment.
Didn't want to formally request changes, but short of an approval are some comments about the option to revert to a uv installed instance of black and the shared linting CI workflow. Unless you opt to move straight to ruff format for formatting, I think it could be a better option than uvx which could be problematic at CI where black may not be installed.
Otherwise, looking good! Nice weaving of POC PR's and other lambda repositories that are utilizing uv and have HTTP server use cases.
| dev = [ | ||
| "coveralls>=4.0.1", | ||
| "ipython>=9.2.0", | ||
| "mypy>=1.15.0", | ||
| "pip-audit>=2.9.0", | ||
| "pre-commit>=4.2.0", | ||
| "pytest>=8.3.5", | ||
| "ruff>=0.11.11", | ||
| ] |
There was a problem hiding this comment.
As mentioned in a side-channel discussion, looks like black may have been missed as dev dependency. Running uv add --dev black should do the trick, and it'll get added here.
.github/workflows/ci.yml
Outdated
| uses: mitlibraries/.github/.github/workflows/python-shared-lint.yml@main | ||
| name: Run linters | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
|
|
||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v7 | ||
|
|
||
| - name: Create virtual environment | ||
| run: make install | ||
|
|
||
| - name: Run black | ||
| run: uvx black --check --diff . | ||
|
|
||
| - name: Run mypy | ||
| run: uv run mypy . | ||
|
|
||
| - name: Run ruff | ||
| run: uv run ruff check . |
There was a problem hiding this comment.
Given the dev dependency for black fix proposed in another comment, I think you could revert back to the shared uv-based linting workflow.
That, itself, basically falls back on make lint, so you can tailor that linting as you see fit for this project (e.g. removing the pip-audit call).
There was a problem hiding this comment.
Yup, full agreement. I had initially forked the workflow to debug if it was older versions being problematic but that was not the case and with your help I've got it working with the shared workflow. We can address updating the workflow to use update checkout and uv-setup separately.
| ### Invoking Lambda via HTTP requests | ||
|
|
||
| The following outlines how to run the Lambda SAM docker image as an HTTP endpoint, accepting requests and returning respnoses similar to a lambda behind an ALB, Function URL, or API Gateway. |
There was a problem hiding this comment.
Nice add! I was concerned at first glance that this SAM README text was heavily focused on an HTTP server use case.... but I reckon that's how this lambda will be used. So, awesome, excellent.
There was a problem hiding this comment.
I promise I just copied this from your PR, I put zero thought into it... so yay you last May!
README.md
Outdated
| Response should have an HTTP status of `200` and respond with: | ||
| ```json | ||
| { | ||
| "response": "pong" | ||
| } | ||
| ``` | ||
|
|
||
| ## Running a Specific Handler Locally with Docker | ||
| ### Invoking Lambda directly | ||
|
|
||
| If this repo contains multiple lambda functions, you can call any handler you copy into the container (see Dockerfile) by name as part of the `docker run` command: | ||
| While Lambdas can be invoked via HTTP methods (ALB, Function URL, etc.), they are also often invoked directly with an `event` payload. To do so with SAM, you do **not** need to first start an HTTP server with `make sam-run`, you can invoke the function image directly: | ||
|
|
||
| ```bash | ||
| docker run -p 9000:8080 my_function:latest lambdas.<a-different-module>.lambda_handler | ||
| ```shell | ||
| echo '{"action": "ping","challenge_secret":"totally-local-archival-packaging"}' | sam local invoke -e - | ||
| ``` | ||
|
|
||
| Response: | ||
| ```text | ||
| {"statusCode": 200, "statusDescription": "200 OK", "headers": {"Content-Type": "application/json"}, "isBase64Encoded": false, "body": "{\"response\": \"pong\"}"} | ||
| ``` | ||
|
|
||
| As you can see from this response, the lambda is still returning a dictionary that _would_ work for an HTTP response, but is actually just a dictionary with the required information. | ||
|
|
||
| It's unknown at this time if APT will get invoked via non-HTTP methods, but SAM will be helpful for testing and development if so. |
There was a problem hiding this comment.
Just noting that some of this is not currently true and/or never will be for this repo, looks like copy/paste from s3-bagit-validator. But it's early days! Not blocking, just noting.
For example, my response from make sam-http-ping:
% make sam-http-ping
curl --location 'http://localhost:3000/myapp' \
--header 'Content-Type: application/json' \
--data '{"msg":"in a bottle"}'
You have successfully called this lambda!%
There was a problem hiding this comment.
Ha, this is copy pasta from the upstream PR which is likely copy pasta from s3-bagit-validator... I'll take a closer look and adjust as appropriate. Thanks for actually reading the words!
| def test_my_function_doesnt_configure_sentry_if_dsn_not_present(caplog, monkeypatch): | ||
| monkeypatch.delenv("SENTRY_DSN", raising=False) | ||
| reload(my_function) | ||
| assert "No Sentry DSN found, exceptions will not be sent to Sentry" in caplog.text | ||
|
|
||
|
|
||
| def test_lambda_handler_missing_workspace_env_raises_error(monkeypatch): | ||
| monkeypatch.delenv("WORKSPACE", raising=False) | ||
| with pytest.raises(RuntimeError) as error: | ||
| my_function.lambda_handler({}) | ||
| assert "Required env variable WORKSPACE is not set" in str(error) |
.github/workflows/ci.yml
Outdated
| run: make install | ||
|
|
||
| - name: Run black | ||
| run: uvx black --check --diff . |
There was a problem hiding this comment.
As noted in a couple of comments for restoring a successful install of black, might be worth revisiting this use of uvx which may not work in CI and/or seeing the comment below of reverting back to the shared workflow.
ghukill
left a comment
There was a problem hiding this comment.
Looking good! Able to run things locally with the black updates.
|
@ghukill would you mind reviewing again? I've properly installed back, reverted to our shared workflow, and updated the docs to be less inaccurate (they'll grow over time, but I believe they are closer to where they should be for starting out) |
ghukill
left a comment
There was a problem hiding this comment.
Approval stands! Looks good for the uv, SAM, python 3.14 base.
Why are these changes being introduced: * `uv` is our preferred way to manage Python dependencies and virtual environments * Python 3.14 is the latest stable release. * Some additional changes were made for developer ergonomics in this cross team repository. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/USE-425 How does this address that need: * Copies most of the changes from the [upstream PR](MITLibraries/python-lambda-template#58) * Updates to python 3.14 * Updates uv to latest * Updates all dependencies * Removes `pip-audit` from pre-commit hooks as we will manage dependency security concners with `renovate` via GitHub integration Document any side effects to this change: * This integrates changes that have not merged into the upstream template repository manually. We may have missed changes or upstream may choose different paths than they were exploring when we copied the changes.
7d10de4 to
57c9ac1
Compare
Why are these changes being introduced:
uvis our preferred way to manage Python dependencies and virtual environmentsRelevant ticket(s):
How does this address that need:
pip-auditfrom pre-commit hooks as we will manage dependency security concners withrenovatevia GitHub integrationDocument any side effects to this change:
Purpose and background context
Describe the overall purpose of the PR changes and any useful background context.
How can a reviewer manually see the effects of these changes?
Explain how to see the proposed changes in the application if possible.
Delete this section if it isn't applicable to the PR.
Includes new or updated dependencies?
YES
Changes expectations for external applications?
NO
What are the relevant tickets?
Code review