Skip to content

[REVIEW] Drop extra copy in get_last_error_text#2044

Open
jakirkham wants to merge 3 commits intorapidsai:mainfrom
jakirkham:drop_xtra_cy_err_cp
Open

[REVIEW] Drop extra copy in get_last_error_text#2044
jakirkham wants to merge 3 commits intorapidsai:mainfrom
jakirkham:drop_xtra_cy_err_cp

Conversation

@jakirkham
Copy link
Copy Markdown
Member

@jakirkham jakirkham commented Apr 29, 2026

In get_last_error_text, the assignment of const char* to bytes results in a copy as bytes must own its memory. After that the bytes object is decoded to a Python str. However it is possible to decode directly from const char* to str avoiding the copy to the temporary bytes object. Hence this code makes that change.

Further Python and Cython def functions always return None if nothing else is returned. Hence the logic can be simplified here to only focus on returning the error text.

Cython can run `decode` directly on `const char*`. So do that instead of
copying to `bytes` first before calling `decode`. This saves a copy.
Python/Cython `def` functions always `return None` if no other `return`
is encountered. So simplify the logic here to only `return` the error
text `str` if is `not NULL`.
@jakirkham jakirkham requested a review from a team as a code owner April 29, 2026 21:38
@jakirkham jakirkham added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Apr 29, 2026
@jakirkham jakirkham changed the title Drop xtra cy err cp Drop extra copy in get_last_error_text Apr 29, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 29, 2026

📝 Walkthrough

Walkthrough

Refactor of get_last_error_text in the exceptions module: the function now decodes and returns the C error string directly when c_err is non-NULL; the previous explicit NULL early-return was removed so the function falls through when c_err is NULL.

Changes

Cohort / File(s) Summary
Exception Error Handling
python/cuvs/cuvs/common/exceptions.pyx
get_last_error_text simplified to decode and return the C error text directly from c_err when non-NULL; removed intermediate bytes variable and the explicit NULL early-return (function now falls through on NULL).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description clearly explains the optimization: eliminating an unnecessary copy by decoding directly from const char* to str, and simplifying control flow by relying on implicit None returns.
Title check ✅ Passed The title accurately describes the main change: dropping an unnecessary intermediate bytes variable copy in the get_last_error_text function, which aligns with the core optimization objective.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@jakirkham
Copy link
Copy Markdown
Member Author

jakirkham commented Apr 30, 2026

Some CI jobs were seeing GitHub API timeouts (for example). So needed to restart CI

@jakirkham jakirkham changed the title Drop extra copy in get_last_error_text [REVIEW] Drop extra copy in get_last_error_text Apr 30, 2026
Copy link
Copy Markdown
Member

@aamijar aamijar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

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

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

2 participants