-
Notifications
You must be signed in to change notification settings - Fork 183
Add PyPI mirror fallbacks #1526
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
Conversation
📝 WalkthroughWalkthroughExposes Changes
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,mts,cts}📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Files:
src/**/*.{ts,mts,cts}📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)src/virtualEnvironment.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (5)
✏️ Tip: You can disable this entire section by setting 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 |
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.
Pull request overview
This PR adds fallback to the default PyPI index when users configure a custom PyPI mirror, addressing package availability issues for mirrors that don't contain all packages (e.g., comfy-kitchen missing from https://pypi.tuna.tsinghua.edu.cn/).
Changes:
- Add
TorchMirrorUrl.DefaultasextraIndexUrlwhenpypiMirroris set - Apply consistently across three install methods that use requirements files
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/constants.ts`:
- Around line 194-201: PYPI_FALLBACK_INDEX_URLS contains an inconsistent SJTU
mirror string without a trailing slash; update the SJTU entry (the element in
the PYPI_FALLBACK_INDEX_URLS array that currently equals
'https://mirror.sjtu.edu.cn/pypi/web/simple') to include the trailing slash so
it matches the other mirror URLs and preserves consistency.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/constants.tssrc/virtualEnvironment.tstests/unit/virtualEnvironment.test.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,mts,cts}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,mts,cts}: Use features available in TypeScript 5.x
Use ESM only (type: module). Avoid CommonJS (require,module.exports)
Target ESNext runtime APIs and syntax. Prefer top-levelawaitonly when it improves clarity
Code must compile withstrictmode andnoImplicitAnyenabled, with zero implicitany
Useunknowninstead ofany. Narrow with type guards.anyis allowed only when interfacing with untyped code and must be localized and justified
Useinterfacefor public object shapes intended for extension/implementation
Usetypefor unions, mapped/conditional types, function types, and when composition is needed
Model domain variants with a discriminant field (e.g.,kind: 'X' | 'Y'). Use exhaustiveswitchstatements
Preferas constobjects and string/number literal unions overenum
Preferreadonlyproperties andReadonlyArray<T>where appropriate. Do not mutate function parameters
PreferT[]overArray<T>for readability
PreferRecord<Key, T>for simple maps; useMap/Setwhen iteration semantics or key types require it
Avoid non-null assertions (!). Use optional chaining, nullish coalescing, and explicit guards
Usesatisfiesto enforce object literal conformance without widening values
Prefer named exports. Default exports are allowed only when a module clearly has a single primary export
Useimport type { X } from '…'for type-only imports. Keep value and type imports separated when helpful for clarity and bundling
Exported functions, classes, and public APIs must have explicit parameter and return types. Internal locals can rely on inference
Prefer arrow functions for local functions and callbacks. Use function declarations when hoisting orthisbinding is required
Modules should be pure. Avoid executing logic at import time unless necessary (e.g., polyfills)
Prefer async/await over rawthen/catchchains. Keep linear flow and usetry/catchfor failures
Eitherawaitpromises or deliberately mark them...
Files:
src/constants.tstests/unit/virtualEnvironment.test.tssrc/virtualEnvironment.ts
src/**/*.{ts,mts,cts}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Centralize reusable domain types in
src/**where discoverable. Avoid ad-hoc inline types for shared structures
Files:
src/constants.tssrc/virtualEnvironment.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: TypeScript: Do not use theanytype; useunknownwhen the type is genuinely unknown
Use JSDoc format for method documentation with common tags@paramand@return(omit@returnforvoidreturn type); use{@link}to reference symbols
Files:
src/constants.tstests/unit/virtualEnvironment.test.tssrc/virtualEnvironment.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use ESLint and Prettier for code formatting; run
yarn lintandyarn formatbefore committing
Files:
src/constants.tstests/unit/virtualEnvironment.test.tssrc/virtualEnvironment.ts
tests/**/*.{ts,mts,cts}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Test code may relax some strictness to maximize ergonomics. Keep type escapes localized
Files:
tests/unit/virtualEnvironment.test.ts
tests/unit/**/*.test.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/vitest.mdc)
tests/unit/**/*.test.{js,ts}: Usevitestfor unit testing
Do not build custom testing infrastructure; use Vitest and existing helpers
Store tests in thetests/unit/directory with directory structure matching that of the tested file insrc/
Tests should be cross-platform compatible; ensure proper use ofpath.normalize,path.join, andpath.sepfor Windows, macOS, and Linux compatibility
Mocks should be cleanly written, easy to understand, and reusable where possible
Prefer the use oftest.extendover loose variables in Vitest tests
Usetestinstead ofitfor defining tests
Files:
tests/unit/virtualEnvironment.test.ts
tests/unit/**/*.{test,spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest for creating unit tests; do not attempt to create custom testing infrastructure
Files:
tests/unit/virtualEnvironment.test.ts
🧠 Learnings (10)
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Leverage TestEnvironment for state manipulation and error simulation instead of manual setup
Applied to files:
tests/unit/virtualEnvironment.test.ts
📚 Learning: 2025-11-25T20:50:25.371Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/vitest.mdc:0-0
Timestamp: 2025-11-25T20:50:25.371Z
Learning: Applies to tests/unit/**/*.test.{js,ts} : Use `vitest` for unit testing
Applied to files:
tests/unit/virtualEnvironment.test.ts
📚 Learning: 2025-11-25T20:50:25.371Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/vitest.mdc:0-0
Timestamp: 2025-11-25T20:50:25.371Z
Learning: Applies to tests/unit/**/*.test.{js,ts} : Do not build custom testing infrastructure; use Vitest and existing helpers
Applied to files:
tests/unit/virtualEnvironment.test.ts
📚 Learning: 2025-12-18T19:46:11.878Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-18T19:46:11.878Z
Learning: Applies to tests/unit/**/*.{test,spec}.ts : Use Vitest for creating unit tests; do not attempt to create custom testing infrastructure
Applied to files:
tests/unit/virtualEnvironment.test.ts
📚 Learning: 2025-11-25T20:50:25.371Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/vitest.mdc:0-0
Timestamp: 2025-11-25T20:50:25.371Z
Learning: Applies to tests/unit/**/*.test.{js,ts} : Prefer the use of `test.extend` over loose variables in Vitest tests
Applied to files:
tests/unit/virtualEnvironment.test.ts
📚 Learning: 2025-11-25T20:50:25.371Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/vitest.mdc:0-0
Timestamp: 2025-11-25T20:50:25.371Z
Learning: Applies to tests/unit/**/*.test.{js,ts} : Tests should be cross-platform compatible; ensure proper use of `path.normalize`, `path.join`, and `path.sep` for Windows, macOS, and Linux compatibility
Applied to files:
tests/unit/virtualEnvironment.test.ts
📚 Learning: 2025-11-25T20:49:50.649Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-25T20:49:50.649Z
Learning: Applies to tests/integration/**/*.{ts,js} : Use `path.join()` and `path.sep` to ensure file path handling works consistently across Windows, macOS, and Linux platforms
Applied to files:
tests/unit/virtualEnvironment.test.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Use Playwright + TypeScript for Electron testing in integration tests
Applied to files:
tests/unit/virtualEnvironment.test.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Import fixtures from testExtensions.ts, not raw Playwright
Applied to files:
tests/unit/virtualEnvironment.test.ts
📚 Learning: 2025-11-25T20:49:40.925Z
Learnt from: CR
Repo: Comfy-Org/desktop PR: 0
File: .cursor/rules/integration-testing.mdc:0-0
Timestamp: 2025-11-25T20:49:40.925Z
Learning: Applies to tests/integration/**/*.spec.ts : Mock native dialogs when needed using app.app.evaluate() with electron.dialog overrides
Applied to files:
tests/unit/virtualEnvironment.test.ts
🧬 Code graph analysis (2)
tests/unit/virtualEnvironment.test.ts (1)
src/virtualEnvironment.ts (1)
getPipInstallArgs(61-93)
src/virtualEnvironment.ts (1)
src/constants.ts (1)
PYPI_FALLBACK_INDEX_URLS(194-201)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-apple-debug-all / build-macos-debug
- GitHub Check: build-and-test-e2e-windows / integration-windows-test
- GitHub Check: lint-and-format (windows-latest)
🔇 Additional comments (7)
src/virtualEnvironment.ts (6)
46-54: LGTM!The interface update from
extraIndexUrl?: stringtoextraIndexUrls?: string[]correctly reflects the new multi-URL capability and follows proper naming conventions.
61-93: LGTM!The function correctly builds pip install arguments, including proper iteration over multiple extra index URLs. The export enables unit testing of this logic.
159-168: Verify the intended behavior for users without a custom mirror.The fallback URLs are only applied when
pypiMirroris configured. Users without a custom mirror won't receive fallbacks. If the intent is also to provide resilience for default PyPI users (e.g., when pypi.org is slow/unavailable), consider whether fallbacks should always be applied.If this is the intended design (fallbacks only for mirror users), this implementation is correct.
413-419: LGTM!Fallback index URLs are correctly integrated into the requirements installation flow.
829-834: LGTM!Consistent with the other installation methods.
861-866: LGTM!ComfyUI Manager requirements installation correctly uses fallback index URLs.
tests/unit/virtualEnvironment.test.ts (1)
155-180: LGTM!The test correctly validates the
getPipInstallArgsfunction with multiple extra index URLs and the index strategy option. The assertion verifies both the correct arguments and their ordering.Consider adding edge case tests for completeness (e.g.,
extraIndexUrls: []orextraIndexUrls: undefined), though the current test covers the primary use case well.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| export const PYPI_FALLBACK_INDEX_URLS: string[] = [ | ||
| 'https://mirrors.aliyun.com/pypi/simple/', | ||
| 'https://mirrors.ustc.edu.cn/pypi/simple/', | ||
| 'https://mirrors.cloud.tencent.com/pypi/simple/', | ||
| 'https://mirrors.huaweicloud.com/repository/pypi/simple/', | ||
| 'https://mirror.sjtu.edu.cn/pypi/web/simple', | ||
| TorchMirrorUrl.Default, | ||
| ]; |
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.
🧹 Nitpick | 🔵 Trivial
Minor inconsistency: trailing slash missing on SJTU mirror URL.
Line 199 (https://mirror.sjtu.edu.cn/pypi/web/simple) lacks a trailing slash, while all other mirror URLs include one. While pip typically handles both forms, consistency is preferable.
✨ Suggested fix
export const PYPI_FALLBACK_INDEX_URLS: string[] = [
'https://mirrors.aliyun.com/pypi/simple/',
'https://mirrors.ustc.edu.cn/pypi/simple/',
'https://mirrors.cloud.tencent.com/pypi/simple/',
'https://mirrors.huaweicloud.com/repository/pypi/simple/',
- 'https://mirror.sjtu.edu.cn/pypi/web/simple',
+ 'https://mirror.sjtu.edu.cn/pypi/web/simple/',
TorchMirrorUrl.Default,
];📝 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.
| export const PYPI_FALLBACK_INDEX_URLS: string[] = [ | |
| 'https://mirrors.aliyun.com/pypi/simple/', | |
| 'https://mirrors.ustc.edu.cn/pypi/simple/', | |
| 'https://mirrors.cloud.tencent.com/pypi/simple/', | |
| 'https://mirrors.huaweicloud.com/repository/pypi/simple/', | |
| 'https://mirror.sjtu.edu.cn/pypi/web/simple', | |
| TorchMirrorUrl.Default, | |
| ]; | |
| export const PYPI_FALLBACK_INDEX_URLS: string[] = [ | |
| 'https://mirrors.aliyun.com/pypi/simple/', | |
| 'https://mirrors.ustc.edu.cn/pypi/simple/', | |
| 'https://mirrors.cloud.tencent.com/pypi/simple/', | |
| 'https://mirrors.huaweicloud.com/repository/pypi/simple/', | |
| 'https://mirror.sjtu.edu.cn/pypi/web/simple/', | |
| TorchMirrorUrl.Default, | |
| ]; |
🤖 Prompt for AI Agents
In `@src/constants.ts` around lines 194 - 201, PYPI_FALLBACK_INDEX_URLS contains
an inconsistent SJTU mirror string without a trailing slash; update the SJTU
entry (the element in the PYPI_FALLBACK_INDEX_URLS array that currently equals
'https://mirror.sjtu.edu.cn/pypi/web/simple') to include the trailing slash so
it matches the other mirror URLs and preserves consistency.
This extends pip install fallback behavior when a PyPI mirror is configured by adding multiple
--extra-index-urlvalues, ending with standard PyPI, so packages missing on TUNA (e.g. comfy-kitchen) still resolve.Fallback order:
┆Issue is synchronized with this Notion page by Unito