Conversation
Summary of ChangesHello @mzueva, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Python environment builder by introducing support for Git-based package dependencies and providing more granular control over the 'pip download' process. It also integrates a new 'parapred' environment, streamlining the setup for projects requiring this specific configuration. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces support for git references in dependencies, adds a new parapred environment, and includes a --no-deps option for package installation. A critical security vulnerability was identified in the package name extraction logic for git URLs, which could lead to command injection on Windows systems due to insecure command execution. This issue requires immediate sanitization. Additionally, the review focused on improving code efficiency and maintainability in the build script.
|
|
||
| glob@10.4.5: | ||
| resolution: {integrity: sha512-7Bv8RF0k6xjo7d4A/PxYLbUCfb6c+Vpd2/mB2yRDlew7Jb5hEXiCD9ibfO7wpk8i4sevK6DFny9h7EYbM3/sHg==} | ||
| deprecated: Old versions of glob are not supported, and contain widely publicized security vulnerabilities, which have been fixed in the current version. Please update. Support for old versions may be purchased (at exorbitant rates) by contacting i@izs.me |
There was a problem hiding this comment.
builder/src/build.ts
Outdated
|
|
||
| const packageName = getPackageName(depSpecClean); | ||
| const packageNameNorm = normalizePackageName(packageName); | ||
| const noDepsList = (config.packages.noDeps || []).map(normalizePackageName); |
There was a problem hiding this comment.
noDepsList doesn’t depend on the current package, but it’s being recomputed on every iteration of for (const depSpec of allDeps). It’s better to move its computation outside the loop (similar to how resolution is computed once on line 258).
There was a problem hiding this comment.
Moved outside of loop
builder/src/build.ts
Outdated
| // Only force source for this package, not its dependencies | ||
| pipArgs.push('--no-binary', packageName); | ||
| if (shouldNoDeps) { | ||
| pipArgs.push('--no-deps'); |
There was a problem hiding this comment.
--no-deps logic is duplicated in three separate branches
The same pattern appears three times. If a fourth download path is added in the future, it's easy to miss. Consider extracting this into buildPipArgs() by passing the package name and the noDeps set, or adding a helper that decorates pip args.
There was a problem hiding this comment.
added to buildPipArgs()
builder/src/build.ts
Outdated
| '--dest', | ||
| destinationDir | ||
| destinationDir, | ||
| '--exists-action', 'w' |
There was a problem hiding this comment.
--exists-action w is a global behavioral change affecting all environments
This flag tells pip to wipe and re-download when a file already exists in the destination directory. Previously, pip would error or prompt in non-interactive mode, which would surface issues like duplicate/conflicting downloads.
With w, conflicts are silently resolved by overwriting. Applying it globally could mask legitimate issues in other environments. Consider either:
- Adding a comment explaining why this is needed globally
- Making it conditional (only for git URL deps)
| "overrides": {}, | ||
| "platformSpecific": {}, | ||
| "resolution": { | ||
| "allowSourceList": ["parapred-pytorch"] |
There was a problem hiding this comment.
The allowSourceList entry here is easy to miss but actually critical — without it git deps just silently don't get installed.
What happens: pip tries to fetch a binary wheel for git+https://... → obviously fails → checks allowSourceList → package not there → skips it. And since strictMissing is false in shared config, there's no error — it just moves on like nothing
happened.
Feels like a trap for the next person who adds a git dependency. They won't know they need to also add it to allowSourceList until they debug why the package is missing at runtime. Maybe worth adding a check in the builder: if a dep starts with
git+ and isn't covered by allowSourceList or forceSource, fail early with a clear message?
| @@ -0,0 +1,5 @@ | |||
| { | |||
… built from source
builder/src/build.ts
Outdated
| // Validate that git URL deps are covered by source-allowing resolution policy. | ||
| // Without allowSourceList/allowSourceAll/forceSource, git deps silently get skipped | ||
| // because the binary wheel attempt always fails and strictMissing defaults to false. | ||
| for (const depSpec of allDeps) { | ||
| const spec = depSpec.trim(); | ||
| if (!spec || !isGitUrl(spec)) continue; | ||
|
|
||
| const name = getPackageName(spec); | ||
| const nameNorm = normalizePackageName(name); | ||
| const coveredByForceSource = shouldForceSource(name, osType, archType) | ||
| || resolution.forceNoBinaryList?.includes(nameNorm); | ||
| const coveredByAllowSource = resolution.allowSourceAll | ||
| || resolution.allowSourceList?.includes(nameNorm); | ||
|
|
||
| if (!coveredByForceSource && !coveredByAllowSource) { | ||
| throw new Error( | ||
| `Git dependency "${spec}" is not in allowSourceList or forceSource. ` + | ||
| `Without this, the package will be silently skipped after binary wheel lookup fails. ` + | ||
| `Add "${name}" to packages.resolution.allowSourceList in config.json.` | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: Please extract the validation logic into a separate function.
Support git references in dependencies, add new parapred environment