Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 214
Fix processing short pip flags in environment dependencies#3708
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
0bf34b03013c11f62f9e35df9ec5fe23869fa7327cFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -78,6 +78,14 @@ func TestNormalizePath_requirementsFile(t *testing.T) { | ||
| assert.Equal(t, "-r requirements.txt", value) | ||
| } | ||
| func TestNormalizePath_environmentDependency(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| location := dyn.Location{File: filepath.Join(tmpDir, "resources", "job_1.yml")} | ||
| value, err := normalizePath("-e ../file.py", location, tmpDir) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, "-e file.py", value) | ||
| } | ||
andrewnester marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| func TestLocationDirectory(t *testing.T) { | ||
| loc := dyn.Location{File: "file", Line: 1, Column: 2} | ||
| dir, err := locationDirectory(loc) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -62,9 +62,9 @@ func jobLibrariesRewritePatterns() []jobRewritePattern { | ||
| dyn.Key("dependencies"), | ||
| dyn.AnyIndex(), | ||
| ), | ||
| TranslateModeEnvironmentRequirements, | ||
| TranslateModeEnvironmentPipFlag, | ||
| func(s string) bool { | ||
| _, ok := libraries.IsLocalRequirementsFile(s) | ||
| _, _, ok := libraries.IsLocalPathInPipFlag(s) | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you make the same change for pipeline environment dependencies? Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't handle any flags there yet, so could be a separate follow-up PR with dedicated changelog entry. ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll do this as a follow-up for this PR | ||
| return !ok | ||
| }, | ||
| }, | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -58,17 +58,11 @@ func IsLibraryLocal(dep string) bool { | ||
| } | ||
| } | ||
| // If the dependency starts with --, it's a pip flag option which is a valid | ||
| // entry for environment dependencies but not a local path | ||
| if containsPipFlag(dep) { | ||
| return false | ||
| } | ||
| // If the dependency is a requirements file, it can either be a local path or a remote path. | ||
| // Even though the path to requirements.txt can be local we don't return true in this function anyway | ||
| // If the dependency starts with - or --, it's a pip flag option which is a valid | ||
| // entry for environment dependencies. Even though the path in the flag can be local we don't return true in this function anyway | ||
| // and don't treat such path as a local library path. | ||
| // Instead we handle translation of these paths in a separate code path in TranslatePath mutator. | ||
| if strings.HasPrefix(dep, "-r") { | ||
| if containsPipFlag(dep) { | ||
| return false | ||
| } | ||
| @@ -80,18 +74,28 @@ func IsLibraryLocal(dep string) bool { | ||
| return IsLocalPath(dep) | ||
| } | ||
| func IsLocalRequirementsFile(dep string) (string, bool) { | ||
| dep, ok := strings.CutPrefix(dep, "-r ") | ||
| if !ok { | ||
| return "", false | ||
| var PipFlagsWithLocalPaths = []string{ | ||
| "-r", | ||
| "-e", | ||
| } | ||
| func IsLocalPathInPipFlag(dep string) (string, string, bool) { | ||
| for _, flag := range PipFlagsWithLocalPaths { | ||
| dep, ok := strings.CutPrefix(dep, flag+" ") | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
| ||
| if ok { | ||
| dep = strings.TrimSpace(dep) | ||
| return dep, flag, IsLocalPath(dep) | ||
| } | ||
| } | ||
| dep = strings.TrimSpace(dep) | ||
| return dep, IsLocalPath(dep) | ||
| return "", "", false | ||
| } | ||
| func containsPipFlag(input string) bool { | ||
| re := regexp.MustCompile(`--[a-zA-Z0-9-]+`) | ||
| // Trailing space means the the flag takes an argument or there's multiple arguments in input | ||
| // Alternatively it could be a flag with no argument and no space after it | ||
| // For example: -r myfile.txt or --index-url http://myindexurl.com or -i | ||
| re := regexp.MustCompile(`--?[a-zA-Z0-9-]+(\s|$)`) | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This made me realize we also need While unlikely to be a valid filename, it is never a flag. ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. true, it should be | ||
| return re.MatchString(input) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -57,6 +57,12 @@ func TestIsLibraryLocal(t *testing.T) { | ||
| {path: "s3://mybucket/path/to/package", expected: false}, | ||
| {path: "dbfs:/mnt/path/to/package", expected: false}, | ||
| {path: "beautifulsoup4", expected: false}, | ||
| {path: "-e some/local/path", expected: false}, | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are some more tricky cases that we should cover here :-/ AI might help identify and handle them Btw, Andrew, please consider how to incorporate such cases. It could be that what you have here is just net-better than what we have now and that we should have a followup with refinements. It could also be that we just need to revise this PR to cover things like ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have quite a good coverage for this overall, but I've added some additional cases for unit / acceptacne tests | ||
| {path: "-i http://myindexurl.com", expected: false}, | ||
| {path: "--index-url http://myindexurl.com", expected: false}, | ||
andrewnester marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| {path: "-i", expected: false}, | ||
| {path: "--index-url", expected: false}, | ||
| {path: "-i -e", expected: false}, | ||
| // Check the possible version specifiers as in PEP 440 | ||
| // https://peps.python.org/pep-0440/#public-version-identifiers | ||
| @@ -129,7 +135,7 @@ func TestIsLocalRequirementsFile(t *testing.T) { | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| got, isLocal := IsLocalRequirementsFile(tt.input) | ||
| got, _, isLocal := IsLocalPathInPipFlag(tt.input) | ||
| require.Equal(t, tt.expected, got) | ||
| require.Equal(t, tt.isLocal, isLocal) | ||
| }) | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR now also handles relative paths for
-e. We can include additional entries for the same PR, for different user-visible changes (eg call out-especifically).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed here fa7327c