chore: enhance import script, add qdrant's docker compose to release artifacts #28
Conversation
WalkthroughAdded Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ImportCLI as Import CLI
participant Qdrant
User->>ImportCLI: start import
ImportCLI->>Qdrant: create collections (HNSW M=0, vectors OnDisk)
ImportCLI->>Qdrant: bulk upload vectors
Note over ImportCLI,Qdrant: Import completes with reduced-RAM indexing
rect rgb(220,240,220)
ImportCLI->>Qdrant: enableProductionIndexing(collection)
Qdrant->>Qdrant: set HNSW M=48, HNSW OnDisk=false
Qdrant->>Qdrant: set quantization AlwaysRam=true
end
Qdrant->>Qdrant: optimizer builds HNSW in background
Qdrant->>ImportCLI: confirmation / warnings
ImportCLI->>User: import complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/import/main.go (1)
241-253: Consider failing fast if production indexing cannot be enabled.Currently, failures in
enableProductionIndexingonly log warnings (line 247), allowing the import to complete successfully even if collections remain in import-optimized state (M=0, vectors on disk). This could severely degrade production query performance.Consider this approach:
// Re-enable production HNSW indexing for all collections log.Println("\n=== Enabling Production HNSW Indexing ===") log.Println("Re-enabling HNSW indexing (M=48) for production queries...") + var productionIndexingErrors []string for _, collectionName := range collections { if err := enableProductionIndexing(ctx, client, collectionName); err != nil { - log.Printf("WARNING: Failed to enable production indexing for %s: %v", collectionName, err) + errMsg := fmt.Sprintf("Failed to enable production indexing for %s: %v", collectionName, err) + log.Printf("ERROR: %s", errMsg) + productionIndexingErrors = append(productionIndexingErrors, errMsg) } } + if len(productionIndexingErrors) > 0 { + log.Fatalf("FATAL: Cannot enable production indexing. Collections are not ready for production queries:\n%s", + strings.Join(productionIndexingErrors, "\n")) + } log.Println("\n✓ Production indexing enabled for all collections.") log.Println("The Qdrant optimizer will build HNSW indexes in the background.") log.Println("Monitor collection stats to track indexing progress.")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/release.yml(1 hunks)cmd/import/main.go(3 hunks)docker-compose.qdrant.yml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/import/main.go (1)
internal/repository/scan_repository_qdrant_impl.go (1)
VectorDim(35-35)
🔇 Additional comments (2)
.github/workflows/release.yml (1)
48-48: LGTM! Docker Compose configuration added to release artifacts.The addition correctly includes the Qdrant Docker Compose configuration in release artifacts, making it easy for users to deploy the optimized Qdrant setup alongside the import tool.
docker-compose.qdrant.yml (1)
10-15: Good alignment with import optimization strategy.The environment variables are well-chosen for bulk import scenarios:
MAX_SEGMENT_SIZE=500000matches the collection configuration incmd/import/main.go(line 304)MAX_OPTIMIZATION_THREADS=4provides reasonable resource limitsWAL_CAPACITY_MB=32enables durability during bulk operationsThe configuration complements the import-time optimizations in the Go code.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/import/main.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/import/main.go (1)
internal/repository/scan_repository_qdrant_impl.go (1)
VectorDim(35-35)
🪛 GitHub Actions: Golang CI Lint
cmd/import/main.go
[error] 389-389: File is not properly formatted (gofumpt).
🪛 GitHub Check: build
cmd/import/main.go
[failure] 389-389:
File is not properly formatted (gofumpt)
🔇 Additional comments (3)
cmd/import/main.go (3)
241-253: Good post-import optimization flow.The approach of re-enabling production HNSW indexing after bulk import completes is sound. Logging failures as warnings (rather than failing the entire import) is acceptable since the data has already been successfully imported, and operators can manually address indexing issues if needed.
Note that if
enableProductionIndexingfails for a collection, that collection will remain in the import-optimized state (M=0, no HNSW), which will result in slower queries. Consider monitoring collection stats to verify that indexing completes successfully.
260-293: Excellent bulk import optimization strategy.The configuration correctly optimizes for bulk import by:
- Disabling HNSW indexing (M=0) to eliminate index build overhead
- Storing vectors on disk (OnDisk=true) to minimize RAM usage
- Allowing quantized vectors on disk (AlwaysRam=false)
This minimizes resource usage during the import phase, with production settings restored by
enableProductionIndexingafterward. The inline comments clearly document the intent.
351-396: Excellent fix for the previous critical bug.The implementation correctly addresses the bug flagged in previous reviews by using
VectorsConfigDiff_ParamsMapto target each named vector individually. The map-based approach ensures that "dirs", "names", and "contents" each receive their own HNSW configuration update.The production settings are well-chosen:
- M=48 for production-quality HNSW graphs
- HNSW in RAM (OnDisk=false) for fast queries
- Vectors on disk (OnDisk=true) to conserve RAM
- Quantization in RAM (AlwaysRam=true) for performance
- IndexingThreshold=0 to trigger immediate background optimization
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/import/main.go (1)
189-205: Fix: invalid range over int prevents build
for workerID := range optimalWorkers.NumWorkerswon’t compile. Use a counted for-loop.- for workerID := range optimalWorkers.NumWorkers { + for workerID := 0; workerID < optimalWorkers.NumWorkers; workerID++ {
♻️ Duplicate comments (2)
cmd/import/main.go (2)
351-395: Named-vector re-enable is now correct (ParamsMap). Please verify client API and run a quick config checkThis fixes the earlier bug by targeting each named vector via ParamsMap — nice.
- Confirm go-client v1.14.0 exposes VectorsConfigDiff_ParamsMap and VectorParamsDiffMap as used here.
- After running, verify each vector’s HNSW config is applied:
#!/bin/bash set -euo pipefail QHOST="${QDRANT_HOST:-localhost}" QPORT="${QDRANT_PORT:-6333}" # HTTP port for REST for c in java_collection python_collection javascript_collection cpp_collection go_collection misc_collection; do echo "Collection: $c" curl -s "http://$QHOST:$QPORT/collections/$c" \ | jq '{name: .result.name, vectors: .result.config.params.vectors}' done
1-824: Ensure gofumpt passes (previous CI failure)Run
gofumpt -w cmd/import/main.gobefore pushing to unblock CI.
🧹 Nitpick comments (4)
cmd/import/main.go (4)
241-251: Post-import production re-enable: good; consider parallelizingThe sequencing and logging are solid. To reduce wall time across many collections, fan out with a small worker pool and collect errors.
I can provide a short diff to parallelize if you want.
260-296: Import-time config looks correct; reduce duplication and confirm Distance/Quantization combo
- Settings align with the two-phase plan (vectors OnDisk, HNSW M=0).
- There’s repeated vector params across dirs/names/contents; extract a helper to avoid drift.
- Sanity-check that Distance_Manhattan + Binary quantization matches your retrieval expectations; keep as-is if this was validated.
Example DRY helper (add once in this file):
func importVectorParams() *qdrant.VectorParams { return &qdrant.VectorParams{ Size: VectorDim, Distance: qdrant.Distance_Manhattan, OnDisk: qdrant.PtrOf(true), HnswConfig: &qdrant.HnswConfigDiff{ M: qdrant.PtrOf(uint64(0)), EfConstruct: qdrant.PtrOf(uint64(500)), FullScanThreshold: qdrant.PtrOf(uint64(100000)), OnDisk: qdrant.PtrOf(true), }, } }Then:
- "dirs": { ... repeated ... }, + "dirs": importVectorParams(), - "names": { ... repeated ... }, + "names": importVectorParams(), - "contents": { ... repeated ... }, + "contents": importVectorParams(),Also applies to: 307-314
56-56: Avoid duplicate VectorDim definitions across packagesYou also define VectorDim=64 in internal/repository. Prefer a single source (e.g., import from internal/repository) to prevent drift.
564-566: Micro-alloc: hoist constant map out of the hot loop
categoryToRankis rebuilt per record. Move it to a package-level or enclosing scope to reduce allocs in large imports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/import/main.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/import/main.go (2)
internal/repository/scan_repository_qdrant_impl.go (1)
VectorDim(35-35)internal/config/config.go (1)
Config(38-72)
Summary by CodeRabbit
New Features
Chores