Repository navigation
Conversation
…th --build-in-source In an npm workspaces monorepo npm keeps one dependency tree, hoisted to the workspace root, and every member's install reconciles that whole tree. Building N functions with --build-in-source therefore reconciled the same tree N times (aws#6567). WorkspaceAwareBuildStrategy wraps the strategy stack outermost, resolves each nodejs function's npm project root (npm's own answer, cached), installs each distinct root once through LambdaBuilder.install_shared_dependencies, and marks the grouped functions with download_dependencies=False - the lever IncrementalBuildStrategy already uses. Scope: Zip nodejs functions on the npm or esbuild workflow, building in source, not cached, not container builds, and only when npm resolves the project root outside the function's own code directory (a workspace member). Everything else, plus any group whose shared install fails or whose aws-lambda-builders does not expose the API, keeps today's per-function install. Sitting outside ParallelBuildStrategy makes the install a barrier: no grouped function build starts before the shared tree is written. Measured on an 8-function esbuild monorepo (~300 packages): 21.9s -> 1.5s; 20 functions: 57.8s -> 3.0s (bench_monorepo.py, median of 3).
…-build-in-source An npm workspaces fixture with two functions holding disjoint dependencies plus a shared workspace package; the lockfile pins lodash 4.17.20 below its ^4.17.20 range so a lockfile-ignoring build is detectable, and the shared package pins lodash 4.17.15 exactly so a hoisting mistake is detectable. An npm shim on PATH logs every invocation, asserting the whole build runs exactly one install (also under --parallel), without --omit=dev, leaving the lockfile bytes untouched; handlers execute from the artifact dirs and see the nested conflicting version; a standalone project keeps its single per-function 'npm update --omit=dev'; --use-container is mutually exclusive with --build-in-source so container builds can never group.
The gate in ApplicationBuilder.build - build_in_source and not cached and not container_manager - is what keeps every other build mode on today's behaviour, and nothing covered it: the grouping strategy had unit tests, its wiring had none. Five tests: --build-in-source wraps the delegate strategy and passes the two npm callbacks; with --parallel the wrapper sits OUTSIDE ParallelBuildStrategy, since the shared install must finish before any grouped function build starts; and --cached, a container build, and a plain build each leave the wrapper out entirely. Verified as guards rather than decoration: weakening the gate to `if self._build_in_source:` fails exactly the --cached and container tests.
aws#6567 is an esbuild report, but the integration coverage was plain npm only. The esbuild workflow reaches the same grouping - both nodejs workflows funnel their installs through the same npm project tree - while its artifact side differs: it bundles what each entry point imports instead of linking a node_modules, so the functions build with no dependency directory at all and the bundle has to have resolved through the tree npm hoisted to the workspace root. The NodeWorkspacesEsbuild fixture carries `esbuild` as the ROOT's own devDependency, which is exactly what the shared install's missing --omit=dev protects, and gives the shared package a conflicting lodash npm keeps nested. The test asserts one install for the whole workspace, that the root's esbuild survived it, that each artifact is a bundle with no node_modules, and that each bundle reports the hoisted 4.17.20 alongside the shared package's nested 4.17.15. The fixture proves its own point: with grouping disabled, the per-function installs prune the root's esbuild and EsbuildBundle fails outright.
_workspace_root_for decides whether a function is a workspace member by comparing npm's reported project root against the code directory, and the two are spelled independently. On Windows two spellings differing only in case - a drive letter included - name the same directory, so reading them as different roots would group a project that is not a workspace at all. normcase is a no-op off Windows; 310 unit and 6 workspace integration tests unchanged.
The workspace grouping changes how every function in an npm workspaces monorepo gets its dependencies, so it ships opt-in: ExperimentalFlag.NodejsMonorepo, enabled by SAM_CLI_BETA_NODEJS_MONOREPO=1, by --beta-features, or by SAM_CLI_BETA_FEATURES=1 with the rest. get_all_experimental() discovers members reflectively, so the new flag registers itself in the beta prompts, the env-var list and telemetry with no other wiring. With the flag off, ApplicationBuilder never wraps WorkspaceAwareBuildStrategy and every function installs for itself - the behaviour of every release so far. Two test changes beyond the new flag-off unit test. The five gating tests now patch the flag ON, so each still tests the gate it names rather than passing because the flag is off; and the integration tests that exercise the grouping ask for --beta-features, including the standalone-project one, where what must keep a per-function install is its own npm root rather than a disabled flag. One new integration test pins the rollout guarantee end to end: the same monorepo fixture with beta features off takes one install per function and still produces correct, disjoint artifacts.
…t hook The call site invoked the hook with the project root alone. aws-lambda-builders#941 makes prepare_shared_project refuse without experimentalNodejsMonorepo - the per-function artifacts link is gated on that same flag, so preparing the root without it would ship artifacts carrying no dependencies. Unflagged, the refusal arrives as a LambdaBuilderError, which this method already catches and turns into a silent fall back to per-function installs: correct artifacts by the slow path, with the feature inert and nothing red to show it. Renamed to follow the hook: install_shared_dependencies became prepare_shared_project upstream, because the same phase for a Cargo workspace compiles every member and installs nothing. The defect survived because nothing tested this method - one assertion pinned the callable into the strategy and stopped there. Adds a test class over what it hands the builders: the enabled flags forwarded, an empty list forwarded rather than omitted, an aws-lambda-builders too old to have the hook declined, and both failure paths falling back. Removing the kwarg again fails two of them.
mypy's no-any-return flagged both helpers: json.loads returns Any against a declared -> dict. Annotating the local satisfies it without a cast or a new import, and keeps the return type the reader wants. Caught by the repository's own lint target, which type-checks tests/ as well as samcli/ - I had run mypy over the two source modules I changed and missed it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue(s) does this change fix?
#6567
Why is this change necessary?
sam build --build-in-sourceon an npm workspaces monorepo is slow, and it gets slower for every function each time any function is added.npm hoists a workspace member's dependencies into one
node_modulesat the monorepo root, so an install run inside any member reconciles the entire tree.sam buildruns that once per function, which makes the install cost O(N × D_total) — quadratic in monorepo growth. Holding one function at exactly one dependency and adding only unrelated siblings around it, that function's own install goes from 0.32s at one function to 2.79s at eight: 8.7× slower to install one unchanged dependency, purely because neighbours exist.How does it address the issue?
One install per workspace root instead of one per function.
WorkspaceAwareBuildStrategygroups the function build definitions by the npm project rootnpm prefixreports, runsLambdaBuilder.prepare_shared_projectonce per distinct root, and then setsdownload_dependencies=Falseon every function in that group. Each function's artifacts are populated from the already-installed shared tree, narrowed to that function's own dependency closure.It wraps outside
ParallelBuildStrategyon purpose: the shared install has to finish before any grouped function build starts.Measured against the only configuration that produces correct artifacts today.
What side effects does this change have?
Opt-in while it rolls out. Gated on
ExperimentalFlag.NodejsMonorepo—--beta-features,SAM_CLI_BETA_NODEJS_MONOREPO=1, orSAM_CLI_BETA_FEATURES=1. With the flag off,ApplicationBuildernever wraps the strategy and every function installs for itself: the behaviour of every release so far, by the same code path. An integration test runs the monorepo fixture with beta features disabled and asserts both the per-function install count and the disjoint artifacts.The same flags go to
prepare_shared_project, which refuses withoutexperimentalNodejsMonorepo. That matters: the per-function artifacts link in aws-lambda-builders is gated on the same flag, so preparing the root without it would ship artifacts carrying no dependencies at all. Enabling one half without the other is now refused rather than silently wrong.Grouping is skipped, with a debug line, for:
--use-containerCodeUri; container builds reach the builders over JSON-RPC--cached.aws-sam/depsand managesdownload_dependenciesitself--build-in-sourceLayerBuildDefinitiongoes through a separate loopThe install's working directory moves from a function's
codeurito the workspace root. That root is inside the user's project and is the directory npm itself would choose; a scope check ("npm prefixresolves outside the codeuri") bounds it, compared throughnormcaseso two spellings of one Windows path match.A failure never fails the build. Anything the preparation raises is caught and the grouped functions fall back to per-function installs.
Mandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesmake update-reproducible-reqsif dependencies were changed — pending the builders release this depends on--build-in-sourcedocs need the workspace-root behaviour and the beta flag; tracked as a follow-upBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.