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.

Sign up for GitHub

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

Get the correct computed tb lineno #158

Merged
merged 3 commits into from
Aug 12, 2024
Merged

Get the correct computed tb lineno #158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b56b946
Get the correct computed tb lineno
jmao-denver Aug 8, 2024
d527107
Work around the breaking change in setuptools
jmao-denver Aug 8, 2024
657f5b9
Minimize the number of calls to get_tb_lineno()
jmao-denver Aug 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/check.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ jobs:
distribution: 'temurin'
java-version: ${{ matrix.java }}

- run: pip install setuptools
- run: pip install "setuptools < 72"
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to limit setuptools?

Copy link
Contributor Author

@jmao-denver jmao-denver Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We reply on setuptools.command.test to run the test suite. See pypa/setuptools#4519 and a solution is out of scope for now. will create a ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#160 is filed.


- name: Run Test
run: python setup.py test
16 changes: 12 additions & 4 deletions src/main/c/jni/org_jpy_PyLib.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2667,7 +2667,7 @@ void PyLib_RedirectStdOut(void)


static const int PYLIB_RECURSIVE_CUTOFF = 3;
#define PyLib_TraceBack_LIMIT 1024
#define PyLib_TraceBack_LIMIT 1024

static PyObject *format_displayline(PyObject *filename, int lineno, PyObject *name)
{
Expand Down Expand Up @@ -2730,6 +2730,13 @@ static int append_to_java_message(PyObject * pyObjUtf8, char **buf, int *bufLen
return 0;
}

static int get_traceback_lineno(PyTracebackObject *tb) {
PyObject* po_lineno = PyObject_GetAttrString((PyObject*)tb, "tb_lineno");
int lineno = (int)PyLong_AsLong(po_lineno);
JPy_DECREF(po_lineno);
return lineno;
}
Comment on lines +2733 to +2738
Copy link
Member

Choose a reason for hiding this comment

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

Took me a little bit to figure out what actually changed. python/cpython#111548 is a useful reference. I'm a bit surprised there's not a more official stable API around PyTracebackObject. I understand the need to wash through PyObject_GetAttrString in this case, but I'm sad.

Does calling PyObject_GetAttrString actually set tb->tb_lineno? I can't tell from the linked PR if that's actually what python does in this case. From the description of the patch

Speed up :obj:Traceback object creation by lazily compute the line number.

it's seems to me that "lazily" means that it will be cached after first access? (If that is not the case, it seems like tb->tb_lineno is completely useless and unused?)

Assuming "yes", we could probably check tb->tb_lineno == -1 before doing the more expensive call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, and PyTracebackObject isn't part of the public stable API, so they are free to change the struct itself and how to use it.


static int format_python_traceback(PyTracebackObject *tb, char **buf, int *bufLen)
{
int err = 0;
Expand All @@ -2752,9 +2759,10 @@ static int format_python_traceback(PyTracebackObject *tb, char **buf, int *bufLe
}
while (tb != NULL && err == 0) {
PyCodeObject* co = PyFrame_GetCode(tb->tb_frame);
int tb_lineno = get_traceback_lineno(tb);
if (last_file == NULL ||
co->co_filename != last_file ||
last_line == -1 || tb->tb_lineno != last_line ||
last_line == -1 || tb_lineno != last_line ||
last_name == NULL || co->co_name != last_name) {
if (cnt > PYLIB_RECURSIVE_CUTOFF) {
pyObjUtf8 = format_line_repeated(cnt);
Expand All @@ -2765,15 +2773,15 @@ static int format_python_traceback(PyTracebackObject *tb, char **buf, int *bufLe
}
}
last_file = co->co_filename;
last_line = tb->tb_lineno;
last_line = tb_lineno;
last_name = co->co_name;
cnt = 0;
}
cnt++;
if (err == 0 && cnt <= PYLIB_RECURSIVE_CUTOFF) {
pyObjUtf8 = format_displayline(
co->co_filename,
tb->tb_lineno,
tb_lineno,
co->co_name);
err = append_to_java_message(pyObjUtf8, buf, bufLen);
if (err != 0) {
Expand Down
Loading