feat: support local package artifacts in requirements#3095
Conversation
|
LGTM |
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds support for staging local wheel/source-archive paths from requirements.txt through the cog_build context before pip install. Overall the approach is clean and well-tested. I found one issue where a valid pip syntax that references local paths can slip through and cause a confusing build failure.
|
|
||
| if strings.HasPrefix(line, "file:") || strings.Contains(line, " @ file:") { | ||
| return "", false, fmt.Errorf("local file URL requirements are not supported: %s", line) | ||
| } |
There was a problem hiding this comment.
Lines like name @ ./pkg.whl (a direct reference without the file: scheme) are silently passed through to pip. Because the file is staged in /tmp/local_package_artifacts/..., pip will fail at build time with a confusing "file not found" error from /tmp instead of a clear message up front.
Please reject these explicitly with a helpful error, similar to how file: URLs are already handled:
| } | |
| if strings.HasPrefix(line, "file:") || strings.Contains(line, " @ file:") { | |
| return "", false, fmt.Errorf("local file URL requirements are not supported: %s", line) | |
| } | |
| if strings.Contains(line, " @ ") { | |
| parts := strings.SplitN(line, " @ ", 2) | |
| urlPart := strings.TrimSpace(parts[1]) | |
| if !isRemoteRequirement(urlPart) && (isLocalPath(urlPart) || hasLocalArtifactSuffix(urlPart)) { | |
| return "", false, fmt.Errorf("local direct reference requirements (\"name @ path\") are not supported; list the path directly instead: %s", line) | |
| } | |
| } | |
| if option, ok := parseUnsupportedLocalOption(line); ok { |
| {name: "LocalRecursiveRequirement", line: "-r requirements-local.txt", expectsErr: true}, | ||
| {name: "InlineHash", line: "./pkg.whl --hash=sha256:abc", expectsErr: true}, | ||
| {name: "LocalDirectory", line: "./pkg", expectsErr: true}, | ||
| } |
There was a problem hiding this comment.
Add test coverage for the direct-reference cases to ensure they are rejected or pass through correctly:
| } | |
| {name: "LocalDirectory", line: "./pkg", expectsErr: true}, | |
| {name: "DirectRefRelative", line: "name @ ./pkg.whl", expectsErr: true}, | |
| {name: "DirectRefAbsolute", line: "name @ /tmp/pkg.whl", expectsErr: true}, | |
| {name: "DirectRefBareRelative", line: "name @ pkg.whl", expectsErr: true}, | |
| {name: "DirectRefRemote", line: "name @ https://example.com/pkg.whl"}, |
|
I'm Bonk, and I've done a quick review of your PR. This PR adds support for staging local wheel/source-archive paths from requirements.txt through the cog_build context before pip install. Overall the approach is clean and well-tested. I found one issue where a valid pip syntax that references local paths can slip through and cause a confusing build failure. Posted a batch review with 2 suggestions on PR #3095: |
Summary
Fixes #1945
Supported examples
Users can list local artifacts directly in their requirements file:
Paths are resolved relative to the requirements file and must point to regular files inside the project. Cog stages those files before the generated
pip install -r /tmp/requirements.txtlayer, without copying the full source tree earlier.Not supported in this PR:
build.run: pip install ./artifact.zipname @ file:...--find-linksdirectories