projects: Correctly build multi-arch Python functions#192
Conversation
goconst detects duplicated constants and forces us to declare them as consts. This is unhelpful when the majority of occurences are in test fixtures. Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
Even though Python is interpreted, Python function images include some platform-specific shared libraries. Previously, we were building only for the host architecture (e.g., arm64 on macOS), which meant we ended up with incompatible shared libraries in the resulting function image. Update the Python SDK builder to produce a layer per architecture, each containing the correct shared libraries for that architecture. Note that the builder image is always the host architecture - we don't want to assume the user has qemu or any other way to run non-native containers. Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPython builds now create and package virtual environments per target architecture, then assemble images with architecture-specific entrypoints. The goconst configuration now ignores test files. ChangesArchitecture-aware Python build
Lint configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant pythonBuilder.Build
participant buildVenv
participant pythonBuildScript
participant ImageAssembly
pythonBuilder.Build->>buildVenv: request venv tars for target architectures
buildVenv->>pythonBuildScript: pass ARCHS
pythonBuildScript-->>buildVenv: create /fn_<pyArch> environments
buildVenv-->>pythonBuilder.Build: return venvTars[arch]
pythonBuilder.Build->>ImageAssembly: append matching venv tar
ImageAssembly-->>pythonBuilder.Build: configure architecture-specific entrypoint
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/project/functions/python.go (2)
228-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
pythonArchitectureis called a second time with the error silently ignored.Line 230 recomputes
pyArchfor each arch even though the same mapping was already validated in the loop at Lines 199–204. The comment acknowledges this, but ignoring the error is fragile — ifpythonArchitectureever has side effects or the architectures list is modified between the two loops, the empty-string fallback would produce a tar path of/fn_with no error.Consider building a
map[string]stringonce and reusing it in both theARCHSenv var and the tar loop:♻️ Suggested refactor
pyArchitectures := make([]string, len(c.Architectures)) + pyArchByGoArch := make(map[string]string, len(c.Architectures)) for i, a := range c.Architectures { pyArchitectures[i], err = pythonArchitecture(a) if err != nil { return nil, err } + pyArchByGoArch[a] = pyArchitectures[i] }ret := make(map[string][]byte) for _, arch := range c.Architectures { - pyArch, _ := pythonArchitecture(arch) // Ignore the error since we already did this once. - ret[arch], err = docker.TarFromContainer(ctx, cid, fmt.Sprintf("/fn_%s", pyArch)) + ret[arch], err = docker.TarFromContainer(ctx, cid, fmt.Sprintf("/fn_%s", pyArchByGoArch[arch])) if err != nil { return nil, errors.Wrapf(err, "failed to retrieve built function for architecture %s", arch) } }🤖 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 `@internal/project/functions/python.go` around lines 228 - 235, Build and retain a map from each architecture to its validated Python architecture value during the initial architecture-processing loop. Update the ARCHS environment construction and the tar retrieval loop in the surrounding function to reuse this map, removing the second pythonArchitecture call and its ignored error while preserving the existing /fn_<architecture> paths.Source: Path instructions
64-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
python3.13is hardcoded in two places that must stay in sync.The build script installs into
/fn_$arch/lib/python3.13/site-packages(Line 68) and the entrypoint points to/fn_%s/lib/python3.13/site-packages/bin/function(Line 265). If the Python version changes — for example when the Debian 13 or distroless base image moves to 3.14 — both paths must be updated together or the entrypoint will point to a nonexistent directory.Consider extracting a constant so the two locations can't drift:
♻️ Suggested refactor
+// pythonVersion is the Python version in the build/runtime images. +// It must match the site-packages path used by both the build script +// and the runtime entrypoint. +const pythonVersion = "3.13" + // At the top of pythonBuildScript, replace the hardcoded version: // --target=/fn_$arch/lib/python3.13/site-packages // becomes (if the script is built via fmt.Sprintf): // --target=/fn_$arch/lib/python%s/site-packages// In configurePythonImage: - cfg.Entrypoint = []string{fmt.Sprintf("/fn_%s/lib/python3.13/site-packages/bin/function", pyArch)} + cfg.Entrypoint = []string{fmt.Sprintf("/fn_%s/lib/python%s/site-packages/bin/function", pyArch, pythonVersion)}Note: the build script is currently a raw string literal, so injecting the constant requires converting it to a
fmt.Sprintfor using string replacement — pick whichever feels cleanest.Also applies to: 265-265
🤖 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 `@internal/project/functions/python.go` around lines 64 - 70, Define a single Python version constant in the surrounding Go code and use it for both the virtualenv pip target in the build script and the entrypoint path currently containing python3.13. Convert the raw build-script string to formatting or equivalent substitution as needed, ensuring both locations derive from the same constant and remain synchronized.
🤖 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 @.golangci.yml:
- Line 123: Update the golangci-lint configuration for version 2.6.2 by removing
the obsolete goconst ignore-tests setting and expressing the test-file exclusion
under linters.exclusions.rules, scoped to goconst and test files.
In `@internal/project/functions/python.go`:
- Line 247: Update the architecture error returned by the Python
architecture-detection logic to include the supported architecture values and,
where appropriate, guidance to use one of them. Preserve the existing
architecture value in the message and modify only the error construction in the
surrounding function.
---
Nitpick comments:
In `@internal/project/functions/python.go`:
- Around line 228-235: Build and retain a map from each architecture to its
validated Python architecture value during the initial architecture-processing
loop. Update the ARCHS environment construction and the tar retrieval loop in
the surrounding function to reuse this map, removing the second
pythonArchitecture call and its ignored error while preserving the existing
/fn_<architecture> paths.
- Around line 64-70: Define a single Python version constant in the surrounding
Go code and use it for both the virtualenv pip target in the build script and
the entrypoint path currently containing python3.13. Convert the raw
build-script string to formatting or equivalent substitution as needed, ensuring
both locations derive from the same constant and remain synchronized.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 63d7f7b1-f104-466a-95af-672a09be5ea5
📒 Files selected for processing (2)
.golangci.ymlinternal/project/functions/python.go
| goconst: | ||
| min-len: 3 | ||
| min-occurrences: 5 | ||
| ignore-tests: true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -i 'golangci-lint|golangci' --glob '!vendor/**' .Repository: crossplane/cli
Length of output: 722
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- .golangci.yml (around line 123) ---'
sed -n '110,135p' .golangci.yml
echo
echo '--- flake.nix (golangci-lint references) ---'
sed -n '150,185p' flake.nix
echo
echo '--- nix/checks.nix (golangci-lint references) ---'
sed -n '35,75p' nix/checks.nix
echo
echo '--- nix/apps.nix (golangci-lint references) ---'
sed -n '35,80p' nix/apps.nixRepository: crossplane/cli
Length of output: 4276
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- flake.lock (nixpkgs inputs) ---'
rg -n '"nixpkgs"|github:NixOS/nixpkgs|rev|locked' flake.lockRepository: crossplane/cli
Length of output: 884
🌐 Web query:
nixpkgs 1766437c5509f444c1b15331e82b8b6a9b967000 golangci-lint package version
💡 Result:
At commit 1766437c5509f444c1b15331e82b8b6a9b967000, which corresponds to the release-25.11 branch of nixpkgs [1][2], the golangci-lint package was at version 2.6.2 [1]. This version of the package was configured to be built using buildGo125Module [1]. As with other versions of golangci-lint in nixpkgs, this package is explicitly tied to a specific Go version to ensure compatibility, as the tool has historically required code changes to support new Go releases [3][1][4].
Citations:
- 1: https://github.com/NixOS/nixpkgs/blob/release-25.11/pkgs/by-name/go/golangci-lint/package.nix
- 2: NixOS/nixpkgs@1766437
- 3: https://github.com/NixOS/nixpkgs/blob/nixos-unstable/pkgs/by-name/go/golangci-lint/package.nix
- 4: golangci-lint: Build with go 1.25 NixOS/nixpkgs#433687
Use linters.exclusions.rules for golangci-lint v2
.golangci.yml:123 is pinned through pkgs.unstable.golangci-lint 2.6.2, so goconst.ignore-tests no longer matches this version. Can you move the test-file exclusion to linters.exclusions.rules so it keeps working?
🤖 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 @.golangci.yml at line 123, Update the golangci-lint configuration for
version 2.6.2 by removing the obsolete goconst ignore-tests setting and
expressing the test-file exclusion under linters.exclusions.rules, scoped to
goconst and test files.
Source: MCP tools
| case "arm64": | ||
| return "aarch64", nil | ||
| default: | ||
| return "", errors.Errorf("unable to determine python architecture for architecture %s", a) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Error message could mention supported architectures.
"unable to determine python architecture for architecture %s" tells the user what failed but not what's supported. Per path instructions, error messages should be meaningful to end users and suggest next steps where possible.
💚 Suggested improvement
-default:
- return "", errors.Errorf("unable to determine python architecture for architecture %s", a)
+default:
+ return "", errors.Errorf("unsupported architecture %s for Python functions: only amd64 and arm64 are supported", a)📝 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.
| return "", errors.Errorf("unable to determine python architecture for architecture %s", a) | |
| return "", errors.Errorf("unsupported architecture %s for Python functions: only amd64 and arm64 are supported", a) |
🤖 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 `@internal/project/functions/python.go` at line 247, Update the architecture
error returned by the Python architecture-detection logic to include the
supported architecture values and, where appropriate, guidance to use one of
them. Preserve the existing architecture value in the message and modify only
the error construction in the surrounding function.
Source: Path instructions
| python3 -m venv /fn_$arch | ||
| /fn_$arch/bin/pip install --platform=manylinux2014_$arch \ | ||
| --only-binary=:all: \ | ||
| --target=/fn_$arch/lib/python3.13/site-packages \ |
There was a problem hiding this comment.
this is the second ref in this file to python3.13, plus the implicit one built into debian and called out in the comment on pythonBuildImage. consider a constant. or a glob?
There was a problem hiding this comment.
I don't think we can use a glob in the Entrypoint since it doesn't run in a shell, declaring the version as a const and interpolating it in both places is a good idea - will make it a little harder to forget to update a place if we ever get a new Python version.
| for arch in $ARCHS ; do | ||
| python3 -m venv /fn_$arch | ||
| /fn_$arch/bin/pip install --platform=manylinux2014_$arch \ | ||
| --only-binary=:all: \ |
There was a problem hiding this comment.
could this cause issues if a function author wants a dep that doesn't ship a 2014 wheel? they're getting a little long in the tooth.
and according to my 🤖 the failure mode is a hard error that's not super informative
ERROR: No matching distribution found
There was a problem hiding this comment.
My understanding of the Python ecosystem is limited, but this makes sense to me. Based on the manylinux README, I've added --platform flags for all the manylinux versions that support amd64 and arm64. The pip docs say that's the way to indicate your runtime target supports multiple versions, and it works properly in my manual testing.
Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>
|
Successfully created backport PR for |
|
Successfully created backport PR for |
3 participants
Description of your changes
Even though Python is interpreted, Python function images include some platform-specific shared libraries. Previously, we were building only for the host architecture (e.g., arm64 on macOS), which meant we ended up with incompatible shared libraries in the resulting function image.
Update the Python SDK builder to produce a layer per architecture, each containing the correct shared libraries for that architecture. Note that the builder image is always the host architecture - we don't want to assume the user has qemu or any other way to run non-native containers.
I have:
./nix.sh flake checkto ensure this PR is ready for review.- [ ] Added or updated unit tests.- [ ] Linked a PR or a docs tracking issue to document this change.backport release-x.ylabels to auto-backport this PR.