fix(bulk_writer): preserve remote upload prefixes on Windows#3658
Conversation
Signed-off-by: VectorPeak <73048950+VectorPeak@users.noreply.github.com>
|
Welcome @VectorPeak! It looks like this is your first PR to milvus-io/pymilvus 🎉 |
|
Tick the box to add this pull request to the merge queue (same as
|
|
/assign |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3658 +/- ##
=======================================
Coverage 93.91% 93.91%
=======================================
Files 72 72
Lines 15487 15487
=======================================
Hits 14545 14545
Misses 942 942 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
XuanYang-cn
left a comment
There was a problem hiding this comment.
Thanks for the focused fix. Please handle the relative local_path regression and run formatting so the lint checks can pass.
Resolve RemoteBulkWriter upload paths against the resolved local data path before computing object-relative paths. Add regression coverage for relative local_path values. Co-authored-by: XuanYang-cn <51370125+XuanYang-cn@users.noreply.github.com> Signed-off-by: VectorPeak <73048950+VectorPeak@users.noreply.github.com>
83e9d34 to
7a20240
Compare
|
Addressed the requested changes from review:
Latest pushed commit: @XuanYang-cn could you please re-review when the checks are available? |
|
Hi @XuanYang-cn, the requested changes have been addressed in the latest signed commit
Please feel free to continue the review when you have time. |
|
@VectorPeak PLZ push an empty commit to re-triggers github's actions. I tried but cannot workaround this github bug. |
Signed-off-by: VectorPeak <73048950+VectorPeak@users.noreply.github.com>
|
Hi @XuanYang-cn, done. I pushed an empty signed commit 11e8b30 to re-trigger GitHub Actions for this PR. I'll keep an eye on the checks as they come back. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: VectorPeak, XuanYang-cn The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
❌ Backport Failed (cc @XuanYang-cn) |
|
✅ Backport Created (cc @XuanYang-cn) |
…indows (#3658) (#3665) Backport of #3658 to `3.0`. Signed-off-by: VectorPeak <73048950+VectorPeak@users.noreply.github.com> Signed-off-by: pymilvus-bot <pymilvus-bot@users.noreply.github.com> Co-authored-by: VectorPeak <garrufariw@gmail.com> Co-authored-by: XuanYang-cn <51370125+XuanYang-cn@users.noreply.github.com>
|
Backport Created: 2.6 -> #3669 |
Cherry-pick of milvus-io#3631 and milvus-io#3658. Backports fix: support struct vector fields in BulkWriter to `2.6`. Backports fix(bulk_writer): preserve remote upload prefixes on Windows to `2.6`. pr: milvus-io#3631 pr: milvus-io#3658 Signed-off-by: pymilvus-bot <pymilvus@zilliz.com>
Normalize RemoteBulkWriter object-key construction so Windows path separators cannot drop the configured remote upload prefix.
What Problem This Solves
RemoteBulkWriter._upload()converts each generated local bulk file into the object key that is sent to S3/MinIO or Azure Blob Storage. That object key is later also stored inbatch_files, so this path is both the upload target and the path Milvus uses for bulk import.The current code derives the local suffix with string replacement and then feeds that suffix back into
Path.joinpath():This mixes two different path domains:
file_pathandsuper().data_pathare local filesystem paths, so on Windows they use\separators.minio_file_pathis an object-storage key, so it should use stable POSIX-style/separators regardless of the client OS.On Windows, a generated file such as:
can produce this suffix after string replacement:
The code only calls
.lstrip("/"), so the leading Windows backslash remains. When that value is passed toPath.joinpath(self._remote_path, "\1.jsonl"), Windows treats"\1.jsonl"as a rooted path. A rooted path resets the join, so the configured remote prefix is discarded.Local reproduction before the fix:
That means a Windows upload configured for:
can instead send and record:
The practical impact is that the blob is uploaded outside the intended
bulk/test/<uuid>/object-key prefix, andbatch_filesrecords the same wrong path for later bulk import. The generated file itself is valid; the bug is in translating a local Windows file path into a remote object-storage key.Change
Build remote object names as POSIX object-key strings instead of host filesystem paths.
The fix keeps the two path domains separate:
Path(...).resolve().relative_to(super().data_path)only to compute the local file path relative to the local bulk writer directory.as_posix()so nested bulk files become stable object-key suffixes such as1.jsonlor1/part.parquetPath.joinpath()After the change, Windows local paths are normalized before they become object-storage keys:
This keeps bucket/container checks, overwrite behavior, supported file extensions, upload client selection,
batch_filesshape, and local cleanup unchanged. It only changes the object-key construction step insideRemoteBulkWriter._upload().Evidence
The focused unit test now verifies the bug boundary directly at the upload call. It creates a
RemoteBulkWriterwithremote_path="bulk/test", writes a local.jsonlfile under the writer's local data directory, and patches the storage calls so no real S3/Azure service is contacted.The test asserts that
_upload_object(object_name=...)receives a remote object key that:uploaded_filesbulk/test/<uuid>/prefix/1.jsonlExpected object key after the fix:
This proves the path translation layer now treats the remote upload target as an object-storage key rather than a host filesystem path.
Possible call chain / impact
The affected path is narrow: only
RemoteBulkWriter's conversion from a local generated file path to the remote S3/Azure object key is changed. In practice, this matters for Windows clients because the local file separator can otherwise leak into object-key construction.Everything around that path stays the same. This PR does not change how bulk files are generated, how schemas or file extensions are validated, how S3/MinIO or Azure clients are initialized, how bucket/container existence and overwrite checks work, or how local files are cleaned up after upload.
Validation
mamba run -n prw-pymilvus-py3.11 python -m pytest tests/unit/test_remote_bulk_writer.py -q- passed, 4 testsmamba run -n prw-pymilvus-py3.11 python -m ruff check pymilvus/bulk_writer/remote_bulk_writer.py tests/unit/test_remote_bulk_writer.py- passedgit diff --check- passed