feat(python): add layers map for separate per-layer dependency sets#13671
feat(python): add layers map for separate per-layer dependency sets#13671nitishagar wants to merge 1 commit into
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis PR adds support for building multiple named Lambda layers from separate Python requirements files via ChangesNamed Python Lambda Layers
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
docs/sf/providers/aws/guide/python.md (1)
342-358: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: combined example block breaks from surrounding doc convention.
The preceding "Lambda Layer" section splits
customandfunctionsconfig into separate YAML blocks; this new example merges both into one, which is a small stylistic inconsistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/sf/providers/aws/guide/python.md` around lines 342 - 358, The new Python layer example in the AWS guide breaks the existing documentation style by merging the `custom` and `functions` sections into a single YAML block. Update the example near the `pythonRequirements` and `api` configuration so it matches the surrounding “Lambda Layer” convention: keep the `custom` configuration and the `functions` configuration as separate YAML snippets while preserving the same `PydanticLambdaLayer` and `WebLambdaLayer` references.packages/serverless/lib/plugins/python/index.js (1)
127-132: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInvalid
options.layersis silently dropped without any warning.When
custom.pythonRequirements.layersis misconfigured (not an object, or an array), it's deleted with no log message. Elsewhere in this same getter, misconfiguration (dockerImage+dockerFile) throws, and docker-related misconfig triggersthis.warningLogged/this.log.warning. A user who accidentally setslayersto the wrong shape will see no layers built and no explanation why.💡 Suggested fix: warn on invalid shape
if ( options.layers != null && (typeof options.layers !== 'object' || Array.isArray(options.layers)) ) { + if (this.log) { + this.log.warning( + 'custom.pythonRequirements.layers must be an object map of layer definitions; ignoring invalid value.', + ) + } else { + this.serverless.cli.log( + 'WARNING: custom.pythonRequirements.layers must be an object map of layer definitions; ignoring invalid value.', + ) + } delete options.layers }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/serverless/lib/plugins/python/index.js` around lines 127 - 132, The invalid options.layers handling in pythonRequirements should not silently delete misconfigured values; update the getter in the Python plugin to emit a warning before removing layers when it is not an object or is an array. Follow the existing validation patterns used nearby in this same getter, such as the dockerImage/dockerFile conflict and docker-related warning logic, and use this.warningLogged or this.log.warning so users know their custom.pythonRequirements.layers setting was ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/serverless/lib/plugins/python/lib/layer.js`:
- Around line 126-131: The empty-requirements fast path in the layer zip flow
only checks requirementsTxtPath with fse.existsSync, so a zero-byte
requirements.txt still falls through and gets packaged. Update the branch in the
layer.js logic that builds the rootZip/writeZip path to treat an empty
requirements.txt the same as a missing one by checking file size as well as
existence, so the JSZip clean empty zip path is used whenever the generated
requirements file has no content.
- Around line 112-165: The symlink creation in zipNamedLayerRequirements is not
awaited, so failures can be swallowed and the layer zip may never be created;
update the async flow to await the file operation and let errors fail the build.
Apply the same fix in zipRequirements as well, since it has the same un-awaited
fse.symlink pattern. Use the existing zipCachePath/targetZipPath branch logic
and keep the Windows copySync path unchanged.
In `@packages/serverless/lib/plugins/python/lib/pip.js`:
- Around line 1067-1079: The empty requirements flow is inconsistent between
installRequirementsForFile and zipNamedLayerRequirements, causing an empty
requirements.txt to be packaged instead of treated as “no reqs.” Update
installRequirementsForFile to signal the empty-file case more explicitly (for
example by returning null/undefined) or update zipNamedLayerRequirements to
check the requirements file size in addition to existsSync before zipping. Make
the behavior consistent using the existing symbols generateRequirementsFile,
installRequirementsForFile, and zipNamedLayerRequirements so an empty layer
produces a clean empty python/ artifact without requirements.txt.
- Around line 1052-1065: Add an explicit guard in the Python requirements layer
flow before `path.isAbsolute(requirementsFile)` is called, since
`requirementsFile` may be missing or non-string when schema validation is
skipped. Update the logic in `pip.js` around the `absRequirementsFile`
resolution to first validate `requirementsFile` and throw a `ServerlessError`
with a clear message and the existing
`PYTHON_REQUIREMENTS_LAYER_REQUIREMENTS_FILE_INVALID` code, rather than allowing
a native `TypeError` to escape. Use the existing `requirementsFile` and
`layerName` handling in this block to keep the error consistent with the current
file-existence check.
---
Nitpick comments:
In `@docs/sf/providers/aws/guide/python.md`:
- Around line 342-358: The new Python layer example in the AWS guide breaks the
existing documentation style by merging the `custom` and `functions` sections
into a single YAML block. Update the example near the `pythonRequirements` and
`api` configuration so it matches the surrounding “Lambda Layer” convention:
keep the `custom` configuration and the `functions` configuration as separate
YAML snippets while preserving the same `PydanticLambdaLayer` and
`WebLambdaLayer` references.
In `@packages/serverless/lib/plugins/python/index.js`:
- Around line 127-132: The invalid options.layers handling in pythonRequirements
should not silently delete misconfigured values; update the getter in the Python
plugin to emit a warning before removing layers when it is not an object or is
an array. Follow the existing validation patterns used nearby in this same
getter, such as the dockerImage/dockerFile conflict and docker-related warning
logic, and use this.warningLogged or this.log.warning so users know their
custom.pythonRequirements.layers setting was ignored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ef6c132-10c6-430e-92b1-c80848a65d3e
📒 Files selected for processing (10)
docs/sf/providers/aws/guide/python.mdpackages/serverless/lib/plugins/python/index.jspackages/serverless/lib/plugins/python/lib/layer.jspackages/serverless/lib/plugins/python/lib/pip.jspackages/serverless/test/unit/lib/plugins/python/layer.test.jspackages/serverless/test/unit/lib/plugins/python/pip.test.jspackages/sf-core/tests/python/test.jspackages/sf-core/tests/python/tests/layers_multi/requirements/pydantic.txtpackages/sf-core/tests/python/tests/layers_multi/requirements/ulid.txtpackages/sf-core/tests/python/tests/layers_multi/serverless.yml
| const absRequirementsFile = path.isAbsolute(requirementsFile) | ||
| ? requirementsFile | ||
| : path.resolve(servicePath, requirementsFile) | ||
|
|
||
| if ( | ||
| !fse.pathExistsSync(absRequirementsFile) || | ||
| !fse.statSync(absRequirementsFile).isFile() | ||
| ) { | ||
| throw new ServerlessError( | ||
| `Python Requirements: layer "${layerName}" requirementsFile "${requirementsFile}" does not exist or is not a file.`, | ||
| 'PYTHON_REQUIREMENTS_LAYER_REQUIREMENTS_FILE_INVALID', | ||
| { stack: false }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
No guard for a missing/undefined requirementsFile before path resolution.
path.isAbsolute(requirementsFile) throws a TypeError if requirementsFile is undefined/non-string. The friendly ServerlessError at lines 1060-1065 only fires once you already have a valid path to check for existence. If schema validation doesn't run (configSchemaHandler?.defineCustomProperties is conditionally guarded in index.js), a layer entry missing requirementsFile will crash with a cryptic native TypeError instead of a clear, actionable message.
🛡️ Suggested guard
const { servicePath, options, serverless } = pluginInstance
+ if (typeof requirementsFile !== 'string' || requirementsFile.length === 0) {
+ throw new ServerlessError(
+ `Python Requirements: layer "${layerName}" is missing a valid requirementsFile.`,
+ 'PYTHON_REQUIREMENTS_LAYER_REQUIREMENTS_FILE_INVALID',
+ { stack: false },
+ )
+ }
const absRequirementsFile = path.isAbsolute(requirementsFile)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const absRequirementsFile = path.isAbsolute(requirementsFile) | |
| ? requirementsFile | |
| : path.resolve(servicePath, requirementsFile) | |
| if ( | |
| !fse.pathExistsSync(absRequirementsFile) || | |
| !fse.statSync(absRequirementsFile).isFile() | |
| ) { | |
| throw new ServerlessError( | |
| `Python Requirements: layer "${layerName}" requirementsFile "${requirementsFile}" does not exist or is not a file.`, | |
| 'PYTHON_REQUIREMENTS_LAYER_REQUIREMENTS_FILE_INVALID', | |
| { stack: false }, | |
| ) | |
| } | |
| const { servicePath, options, serverless } = pluginInstance | |
| if (typeof requirementsFile !== 'string' || requirementsFile.length === 0) { | |
| throw new ServerlessError( | |
| `Python Requirements: layer "${layerName}" is missing a valid requirementsFile.`, | |
| 'PYTHON_REQUIREMENTS_LAYER_REQUIREMENTS_FILE_INVALID', | |
| { stack: false }, | |
| ) | |
| } | |
| const absRequirementsFile = path.isAbsolute(requirementsFile) | |
| ? requirementsFile | |
| : path.resolve(servicePath, requirementsFile) | |
| if ( | |
| !fse.pathExistsSync(absRequirementsFile) || | |
| !fse.statSync(absRequirementsFile).isFile() | |
| ) { | |
| throw new ServerlessError( | |
| `Python Requirements: layer "${layerName}" requirementsFile "${requirementsFile}" does not exist or is not a file.`, | |
| 'PYTHON_REQUIREMENTS_LAYER_REQUIREMENTS_FILE_INVALID', | |
| { stack: false }, | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/serverless/lib/plugins/python/lib/pip.js` around lines 1052 - 1065,
Add an explicit guard in the Python requirements layer flow before
`path.isAbsolute(requirementsFile)` is called, since `requirementsFile` may be
missing or non-string when schema validation is skipped. Update the logic in
`pip.js` around the `absRequirementsFile` resolution to first validate
`requirementsFile` and throw a `ServerlessError` with a clear message and the
existing `PYTHON_REQUIREMENTS_LAYER_REQUIREMENTS_FILE_INVALID` code, rather than
allowing a native `TypeError` to escape. Use the existing `requirementsFile` and
`layerName` handling in this block to keep the error consistent with the current
file-existence check.
Add `custom.pythonRequirements.layers` — a named map where each entry specifies its own requirements file. For each entry the plugin: 1. Installs the explicit requirements file into a per-layer working directory via a new `installRequirementsForFile` helper in pip.js. 2. Zips the installed packages under a `python/` prefix to `.serverless/pythonRequirements-<name>.zip`. 3. Registers `service.layers[<name>]` with `package.artifact` set so the core layer compiler emits `<Name>LambdaLayer` and `<Name>LambdaLayerQualifiedArn` without further changes. Functions attach a layer with `Ref: <Name>LambdaLayer`. Invariants preserved: - The existing single-layer (`layer: true`) path is untouched. - `layer` and `layers` coexist independently. - The reserved name `pythonRequirements` and collisions with existing service layers are rejected before any install work begins. - Registration is deferred to a post-loop `Object.assign` so a mid-loop failure never leaves a half-registered state. Adds a JSON schema for `custom.pythonRequirements.layers` (via `defineCustomProperties`) and extends the function-less-service hook guard to keep hooks alive for `layers`-only services. Closes serverless#13384
8df2d23 to
60f04e1
Compare
Summary
custom.pythonRequirements.layers— a named map where each entry specifies its own requirements file, enabling independent dependency sets across multiple Lambda layers.service.layers[<name>]entry; the core compiler emits<Name>LambdaLayer+<Name>LambdaLayerQualifiedArnwith no changes to core code.layer: true) path is untouched;layerandlayerscoexist independently.Key design decisions
service.layerswhich is already a named map; makesRef: <Name>LambdaLayernatural.package.artifactexplicit pointer: satisfiesprovider.js,get-lambda-layer-artifact-path.js, andpackage-service.jswithout relying on naming-convention coincidence.Object.assign-ed ontoservice.layersonly after all layers build successfully — no half-registered state on failure (INV-8).pythonRequirementsand collisions with existing service layers are rejected before any install work begins (INV-3).Test plan
npm run test:unit -w @serverless/framework -- test/unit/lib/plugins/python— 12 new unit tests cover all invariants (registration, defaults, overrides, empty map, reserved name, collision, mid-loop failure atomicity, single-layer coexistence)npm run test:unit -w @serverless/framework— 1887 tests, all passpackages/sf-core/tests/python/tests/layers_multi/verifies end-to-end packaging with a real Python toolchain (best-effort; not required for CI without Python)npm run prettier:fix && npm run lint:fix— cleanCloses #13384
Summary by CodeRabbit