Skip to content

Conversation

@bcalvert-graft
Copy link

Description

As per the discussion from #2221, the pattern,

try:
    <do something that might raise Exception>
except ExceptionType as exc
    <maybe do some additional stuff here>
    raise exc from exc

can break downstream tooling that seeks to unroll exc.__cause__ (e.g. for processing the stack trace).

This PR tweaks all spots in the pymilvus codebase that were using that pattern to instead either just use this pattern

try:
    <do something that might raise Exception>
except ExceptionType as exc
    <maybe do some additional stuff here>
    raise

or, if the try/except didn't have any additional logic in the except block, I removed the try/except entirely as a direct reraise would be redundant in those cases.

NB As mentioned in #2221, I originally only noticed three spots where the raise err from err pattern was used. While working on this PR I found a number of others and I ended up changing all of them. As per @XuanYang-cn's comment, if there are important reasons for one or more of these spots to stay as-is, please let me know and I can revert the changes to that section accordingly.

Testing

Finding all the spots

To find all the spots, I ran this grep

$ grep "raise \(.*\) from \1" pymilvus/**/*.py 

On master branch, we get a non-zero number of hits with the expected pattern. On the branch for this PR, the grep returns no values

Testing the fix

In one process, run

In [1]: from milvus_lite.server import Server

In [2]: s = Server('test.db', '127.0.0.1:9999')

In [3]: s.start()
Out[3]: True

In [4]: s._p.wait()

In another process, run

In [1]: import pymilvus

In [2]: client = pymilvus.MilvusClient("http://localhost:9999")
   ...: collection_name = "fake_embeddings"
   ...: if client.has_collection(collection_name=collection_name):
   ...:     client.drop_collection(collection_name=collection_name)
   ...: 

In [3]: vector_dim = 512
   ...: client.create_collection(
   ...:     collection_name=collection_name,
   ...:     vector_field_name="vector",
   ...:     dimension=vector_dim,
   ...:     auto_id=True,
   ...:     enable_dynamic_field=True,
   ...:     metric_type="COSINE",
   ...: )

In [4]: import numpy as np

In [5]: vectors = np.random.normal(loc=0, scale=1, size=(vector_dim + 2,))

In [6]: try:
   ...:     client.insert(collection_name, {"vector": vectors})
   ...: except pymilvus.MilvusException as exc:
   ...:     assert exc.__cause__ is not exc, "Exception is its own cause"

The In [6]: step will not through an AssertionError

@sre-ci-robot sre-ci-robot requested a review from czs007 August 9, 2024 16:58
@sre-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bcalvert-graft
To complete the pull request process, please assign xuanyang-cn after the PR has been reviewed.
You can assign the PR to them by writing /assign @xuanyang-cn in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sre-ci-robot
Copy link

Welcome @bcalvert-graft! It looks like this is your first PR to milvus-io/pymilvus 🎉

@yunmanger1
Copy link

Can we get this fix anytime soon? This breaks our exception handling forcing to make some workarounds

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants