-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Merge/quant into main #6679
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
Merge/quant into main #6679
Conversation
…ants and Qdrant client
choronz
left a 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.
done
| export const MIN_CHUNK_REMAINDER_CHARS = 200 // Minimum characters for the *next* chunk after a split | ||
| export const MAX_CHARS_TOLERANCE_FACTOR = 1.15 // 15% tolerance for max chars | ||
|
|
||
| <<<<<<< HEAD |
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.
Merge conflict markers (<<<<<<<, =======, >>>>>>>) remain in the file. Remove these and decide on the correct value for MAX_CHARS_TOLERANCE_FACTOR.
| export const MAX_LIST_FILES_LIMIT_CODE_INDEX = 50_000 | ||
| export const BATCH_SEGMENT_THRESHOLD = 60 // Number of code segments to batch for embeddings/upserts | ||
| export const MAX_LIST_FILES_LIMIT_CODE_INDEX = 10_000 | ||
| export const BATCH_SEGMENT_THRESHOLD = 8 // 60 Number of code segments to batch for embeddings/upserts |
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.
BATCH_SEGMENT_THRESHOLD is set to 8 but the comment still indicates '60 Number of code segments'. Update the comment to accurately reflect the current threshold.
| export const BATCH_SEGMENT_THRESHOLD = 8 // 60 Number of code segments to batch for embeddings/upserts | |
| export const BATCH_SEGMENT_THRESHOLD = 8 // 8 Number of code segments to batch for embeddings/upserts |
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.
Thank you for your contribution! I've reviewed the changes and found several critical issues that need attention before this can be merged.
Critical Issues (Must Fix):
-
Missing PR Documentation: The PR template is not filled out - there's no linked issue, no description of changes, and no test procedure. This makes it difficult to understand the purpose and impact of these changes.
-
Workflow Breaking Change (.github/workflows/publish-extension.yml):
- The trigger was changed from
pull_requesttopush, but the job conditions still check forgithub.event_name == 'pull_request'. This creates an impossible condition where the workflow will never run. - The actual publishing step has been completely removed. Without it, this workflow only packages and tags but doesn't actually publish to the marketplace.
- The trigger was changed from
-
Performance Impact Without Justification (src/services/code-index/constants/index.ts):
- MAX_LIST_FILES_LIMIT_CODE_INDEX: 50,000 → 10,000 (80% reduction)
- BATCH_SEGMENT_THRESHOLD: 60 → 8 (87% reduction)
- MAX_BATCH_TOKENS: 100,000 → 16,384 (84% reduction)
- MAX_ITEM_TOKENS: 8,191 → 2,048 (75% reduction)
These changes could severely impact performance for users with large codebases.
Important Suggestions (Should Consider):
-
Node.js Major Version Upgrade (package.json): Upgrading from Node.js 20.19.2 to 22.17.0 is a major version change that could introduce breaking changes. Has this been tested across all environments?
-
Quantization Configuration (src/services/code-index/vector-store/qdrant-client.ts): Adding binary quantization without context about why this is needed or what performance/accuracy trade-offs were considered.
-
Confusing Code Comment: Line 6 in constants/index.ts has "8% 15 tolerance" which appears to be a typo.
Please address these issues and provide context for these significant changes.
Related GitHub Issue
Closes: #
Roo Code Task Context (Optional)
Description
Test Procedure
Pre-Submission Checklist
Screenshots / Videos
Documentation Updates
Additional Notes
Get in Touch
Important
Updates extension publishing workflow, modifies code index constants, and adds quantization config to Qdrant vector store.
.github/workflows/marketplace-publish.ymltopublish-extension.yml.pull_requesttopushinpublish-extension.yml.22.17.0inpackage.json.MAX_LIST_FILES_LIMIT_CODE_INDEXto10,000andBATCH_SEGMENT_THRESHOLDto8inconstants/index.ts.MAX_BATCH_RETRIESto5andINITIAL_RETRY_DELAY_MSto600inconstants/index.ts.MAX_ITEM_TOKENSto2048inconstants/index.ts.quantization_configtoQdrantVectorStoreinqdrant-client.ts.This description was created by
for 0cdb466. You can customize this summary. It will automatically update as commits are pushed.