fix(async): make AsyncMilvusClient.refresh_load actually refresh (#3772) - #3773
fix(async): make AsyncMilvusClient.refresh_load actually refresh (#3772)#3773Anai-Guo wants to merge 3 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Anai-Guo The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
…vus-io#3772) AsyncMilvusClient.refresh_load() delegated to AsyncGrpcHandler.refresh_load(), which issues a single GetLoadingProgress RPC and returns refresh_progress. It polls progress and never asks the server to refresh anything. The synchronous twin MilvusClient.refresh_load() re-issues LoadCollection with refresh=True and waits for the refresh to complete. Because optimize_collection() is structured identically in both clients and ends with a refresh_load() call, the async variant reported ProgressStage.REFRESHING_LOAD and status="success" while the compacted segments were never reloaded. Route refresh_load() through the async load primitives instead, mirroring the sync client: load_collection(_refresh=True), or load_partitions(_refresh=True) when partition_names is given. Both already forward _refresh through Prepare and wait via wait_for_loading_collection / wait_for_loading_partitions. A str partition name is normalized to a list, matching load_partitions/release_partitions. AsyncGrpcHandler.refresh_load() is left untouched; reading refresh progress is a reasonable low-level operation, only the client wiring was wrong. The async unit test asserted delegation by handler method name only, so it stayed green regardless of what the handler did. It now asserts the same contract the sync test does (_refresh=True on the load call), plus partition scoping. Signed-off-by: Anai-Guo <antai12232931@outlook.com>
5aad3e8 to
8921139
Compare
|
Tick the box to add this pull request to the merge queue (same as
|
| self, | ||
| collection_name: str, | ||
| partition_names: Optional[List[str]] = None, | ||
| partition_names: Optional[Union[str, List[str]]] = None, |
There was a problem hiding this comment.
pymilvus/milvus_client/async_milvus_client.py line:864
Low ---- This method public contract changed (returns None instead of the old refresh_progress int, and partition_names now accepts str or List[str]) but it has no docstring, so the new contract is not discoverable from code. Callers that read the old return value break silently; consider adding a short docstring documenting the None return and the partition_names union so future maintainers know the change is deliberate.
| @pytest.mark.asyncio | ||
| async def test_refresh_load_requests_a_refresh(self): | ||
| client, handler = _make_client() | ||
| await client.refresh_load("col") |
There was a problem hiding this comment.
tests/unit/test_async_milvus_client_ops.py line:125
Low ---- These tests lock the handler calls but not the public return contract this PR deliberately changes (None instead of the old refresh_progress int). Consider asserting result = await client.refresh_load("col"); assert result is None here so the new contract cannot silently regress.
Signed-off-by: Anai-Guo <antai12232931@outlook.com>
Signed-off-by: Anai-Guo <antai12232931@outlook.com>
|
Thanks @yhmo — both points addressed in e36271c:
🤖 Generated with Claude Code |
9c28268 to
6f6f048
Compare
Fixes #3772
What
AsyncMilvusClient.refresh_load()delegated toAsyncGrpcHandler.refresh_load(), whichissues a single
GetLoadingProgressRPC and returnsresponse.refresh_progress. That is aread-only poll — it never asks the server to refresh anything.
The synchronous twin does the real work:
LoadCollection(refresh=True)followed bywait_for_loading_collection(is_refresh=True).Why it matters
optimize_collection()exists in both clients and is structured line-for-line the same.Both end with:
(async
async_milvus_client.py:2415, syncmilvus_client.py:3074)On the sync client the compacted segments are reloaded before the call returns. On the async
client nothing is reloaded —
optimize_collection()still reportsProgressStage.REFRESHING_LOADand returnsstatus="success"while queries keep being servedfrom the pre-compaction segments. Same for a direct
await client.refresh_load(name)after abulk import. No error, no warning.
partition_nameswas affected too: the async signature accepted it and forwarded it toGetLoadingProgress, so it selected which progress was read rather than what got refreshed.The fix
Route
refresh_load()through the async load primitives, mirroring the sync client:partition_namesgiven →conn.load_partitions(..., _refresh=True)conn.load_collection(..., _refresh=True)Both already exist on
AsyncGrpcHandler(async_grpc_handler.py:1321and:399), alreadyforward
_refreshthroughPrepare, and already wait viawait_for_loading_partitions/wait_for_loading_collection. They were simply never called fromrefresh_load. Astrpartition name is normalized to a list, matching
load_partitions/release_partitionsonthe same client.
Deliberately not in scope:
AsyncGrpcHandler.refresh_load()is left exactly as it is.Reading refresh progress is a reasonable low-level operation; only the name and the client
wiring were wrong. Renaming it (e.g.
get_refresh_progress) would change a public handlermethod and belongs in a separate call by maintainers.
Behavior note for review: the client method now returns
None, like the sync twin,instead of an integer. The previous return value was the progress of a refresh that was never
triggered, so it carried no usable information — but flagging it explicitly in case you would
rather preserve the old signature.
Why CI did not catch it
The async test asserted delegation by handler method name only
(
tests/unit/test_async_milvus_client_ops.py,_SIMPLE_ASYNC_DELEGATION_CASES):That stays green no matter what the handler does. The sync test asserts the actual contract
(
tests/unit/test_milvus_client.py::test_refresh_load:_refresh is Trueon theload_collectioncall), so the sync side was protected and the async side was not. This PRbrings the async test up to the same contract and adds partition scoping.
Verification
Red/green on the new tests, same tree, only
async_milvus_client.pyreverted for the red run:Full unit suite on this tree: 4486 passed, 1 skipped, 2 failed. Both failures
(
orm/test_iterator.py::...test_deleted_cp_file_is_recreated_during_iterationandtest_check.py::TestGetCommit::test_get_commit) reproduce identically with the whole changestashed, i.e. they are pre-existing on this machine and unrelated. Six
test_bulk_*/test_version.pymodules fail to collect locally for missing optional deps (minio,azure,setuptools_scm) and were excluded.ruff checkandruff format --checkclean on both changed files (ruff 0.15.11, within theruff>=0.12.9,<1pin inpyproject.toml).Not run locally: any test needing a live Milvus server (no server on this machine), so the
refresh RPC itself is covered by mock-level assertions only.
🤖 Generated with Claude Code