Skip to content
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

Change raise e from e calls to be just raise and remove unneeded ones #2225

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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