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

fix: DECREFing PyObject*s from Python C API calls in v1. #2311

Merged
merged 12 commits into from
Mar 13, 2023

Conversation

jpivarski
Copy link
Member

A memory leak in Coffea was traced to this: we were calling Python C API functions (just PyUnicode_DecodeUTF8) that returned new refs and then passing them directly to pybind11 py::object (or its subclasses') constructors. I had thought that the py::object constructor would steal this reference and implicitly call Py_DECREF on it when the py::object enters its destructor, but this is evidently not the case. (The memory leak in #2310 toggles exactly with adding Py_DECREF into parameters2dict.)

In this PR, I have added Py_DECREF calls as late as possible in every branch of the program flow on the raw PyObject* objects directly returned by PyUnicode_DecodeUTF8. I got all of these cases, and there are no other Python C API function calls that return PyObject* in the codebase. All of the other Python C API functions called,

return C types, not PyObject*. (The Python documentation says the char* that PyBytes_AsString returns is owned by Python and must not be released. In our code, we copy it immediately.)

This PR also updates the version number in anticipation of an Awkward v1 bug-fix release, and it fixes a test that now fails a NumPy deprecation warning (no relationship to the actual bug; it's just that main-v1 is an old branch).

There is no equivalent action to be performed on Awkward main because none of this stuff exists there.

@jpivarski jpivarski linked an issue Mar 13, 2023 that may be closed by this pull request
@jpivarski
Copy link
Member Author

jpivarski commented Mar 13, 2023

Check: are the following memory leaks now fixed?

I don't remember why I thought that #567 was an unresolved memory leak. (The first part of that issue addressed a different leak in ArrayBuilder; afterward, I had thought we had hints of another one, but the conversation that's there doesn't look conclusive to me now.)

@jpivarski
Copy link
Member Author

@agoose77 I disabled tests of ak.to_/from_rdataframe and JAX. The RDataFrame tests were testing the last ak._v2.* version, and the JAX tests were testing the v1 version, neither of which are supported anymore. Hopefully, the remaining tests will pass.

Do you expect that enough of the infrastructure will work to do a PyPI 1.10.3 release the normal way—by creating a release in GitHub targeting the main-v1 branch (after this gets merged)?

What would be necessary to get it from PyPI into conda-forge? Does that pick up any new PyPI releases, even if they're out of order?

@jpivarski jpivarski linked an issue Mar 13, 2023 that may be closed by this pull request
@jpivarski jpivarski linked an issue Mar 13, 2023 that may be closed by this pull request
@agoose77
Copy link
Collaborator

I think the PyPI release should work just fine with the wheels.yml workflow. I'm not so sure about how we would release a patch version for an old major version on conda-forge. I think it should be possible, but let me do some digging.

src/python/content.cpp Outdated Show resolved Hide resolved
src/python/content.cpp Outdated Show resolved Hide resolved
@agoose77
Copy link
Collaborator

I checked on Matrix, and it looks like we just need to maintain a v1 branch on our feedstock. That should take care of it. Once we make the PyPI release, I can revert back to the v1 feedstock for a branch to make the release.

@jpivarski
Copy link
Member Author

Alternative to using the py:: namespace in front of reinterpret_steal:

#include <pybind11/pytypes.h>

But I like the namespace to denote all things pybind11.

@codecov
Copy link

codecov bot commented Mar 13, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main-v1@198cf61). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 2b6d805 differs from pull request most recent head 7161d3d. Consider uploading reports for the commit 7161d3d to get more accurate results

Additional details and impacted files

Comment on lines -79 to -112
assert to_list(wone + wtwo) == [
[
{
"x": 0.9524937500390619,
"y": 1.052493750039062,
"weight": 2.831969279439222,
},
{"x": 2.0, "y": 2.2, "weight": 5.946427498927402},
{
"x": 2.9516640394605282,
"y": 3.1549921183815837,
"weight": 8.632349833200564,
},
],
[],
[
{
"x": 3.9515600270076154,
"y": 4.206240108030463,
"weight": 11.533018588312771,
},
{"x": 5.0, "y": 5.5, "weight": 14.866068747318506},
],
]
assert to_list(abs(one)) == [
[1.4866068747318506, 2.973213749463701, 4.459820624195552],
[],
[5.946427498927402, 7.433034373659253],
]
assert to_list(one.distance(wtwo)) == [
[0.14142135623730953, 0.0, 0.31622776601683783],
[],
[0.4123105625617664, 0.0],
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did we get rid of this because it's flukey?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. It's been failing in the last digits on only some platforms. This is a test of the not-completely-done v2 inside ak._v2.*, so I don't think we need to work very hard to save it.

Copy link
Member Author

Choose a reason for hiding this comment

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

In principle, we could remove all of ak._v2 and tests/v2 from main-v1, but I don't want to make such a large diff in this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer to keep some of the tests so we don't fully regress, but I agree this is transitional code and should be treated as such. We didn't make long-term guarantees about v2-in-v1.

@jpivarski jpivarski merged commit 3375b4a into main-v1 Mar 13, 2023
@jpivarski jpivarski deleted the jpivarski/awkward-v1-Python-C-API-DECREFs branch March 13, 2023 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants