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: handle unhashable behaviour type #2770

Merged
merged 11 commits into from
Oct 26, 2023

Conversation

ianna
Copy link
Collaborator

@ianna ianna commented Oct 25, 2023

fixes #2762

@ianna ianna requested a review from jpivarski October 25, 2023 11:16
@ianna ianna marked this pull request as draft October 25, 2023 11:34
@ianna ianna force-pushed the ianna/support_non-default_behaviours branch from c71a494 to a6650d6 Compare October 25, 2023 11:50
@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Merging #2770 (2716f8f) into main (c310ec1) will decrease coverage by 0.02%.
Report is 1 commits behind head on main.
The diff coverage is 88.88%.

Additional details and impacted files
Files Coverage Δ
src/awkward/_connect/numba/builder.py 85.02% <100.00%> (+0.06%) ⬆️
src/awkward/_connect/numba/layoutbuilder.py 87.24% <83.33%> (-0.03%) ⬇️

... and 73 files with indirect coverage changes

@ianna ianna temporarily deployed to docs October 25, 2023 12:03 — with GitHub Actions Inactive
@ianna ianna marked this pull request as ready for review October 25, 2023 12:32
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

This was already dealt with in

def dict2serializable(obj):
if obj is None:
return None
else:
return tuple(obj.items())
def serializable2dict(obj):
if obj is None:
return None
else:
return dict(obj)

Perhaps ArrayBuilder could use the same mechanism as ArrayView?

Also, @agoose77 was going to send an example of a behavior that can't be serialized due to a lambda function. I hope we don't have to pull in cloudpickle just for that. But maybe Numba's serialization already does something clever: we should try it and find out.

@agoose77
Copy link
Collaborator

Here's a non-pickleable array:

import pickle

import numpy as np
import numba as nb
import pytest

import awkward as ak


def make_add_xyr():
    def add_xyr(left, right):
        x = left.x + right.x
        y = left.y + right.y
        return ak.zip(
            {
                "x": x,
                "y": y,
                "r": np.sqrt(x**2 + y**2),
            },
            with_name="xyr",
        )

    return add_xyr


behavior = {(np.add, "xyr", "xyr"): make_add_xyr()}


a = ak.Array([{"x": 3, "y": 4, "r": 5}], with_name="xyr", behavior=behavior)
b = ak.Array([{"x": 3, "y": 4, "r": 5}], with_name="xyr", behavior=behavior)

builder = ak.ArrayBuilder(behavior=behavior)


with pytest.raises(AttributeError):
    behavior_pkl = pickle.dumps(behavior)


@nb.njit
def make_ab(builder):
    builder.begin_record("xyz")
    builder.field("x").integer(3)
    builder.field("y").integer(4)
    builder.field("z").integer(3)
    builder.end_record()

    builder.begin_record("xyz")
    builder.field("x").integer(3)
    builder.field("y").integer(4)
    builder.field("z").integer(3)
    builder.end_record()
    return builder


result = make_ab(builder).snapshot()

print(result)

We discussed in the meeting perhaps stashing a reference in the Python layer, and retrieving it later on? Just in case we forgot this (though I doubt we did).

@ianna ianna force-pushed the ianna/support_non-default_behaviours branch from 3aa4159 to fc1c306 Compare October 26, 2023 07:21
Copy link
Collaborator Author

@ianna ianna left a comment

Choose a reason for hiding this comment

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

@agoose77 - I'm done with the PR. Please, check and modify if needed. For example, I put both tests in one - it could be split. Thanks!

@ianna ianna requested a review from jpivarski October 26, 2023 07:31
@ianna ianna marked this pull request as draft October 26, 2023 07:33
@ianna
Copy link
Collaborator Author

ianna commented Oct 26, 2023

@agoose77 - I'm done with the PR. Please, check and modify if needed. For example, I put both tests in one - it could be split. Thanks!

Nope, not quite finished with it :-(

@ianna ianna temporarily deployed to docs October 26, 2023 07:37 — with GitHub Actions Inactive
@ianna ianna temporarily deployed to docs October 26, 2023 08:47 — with GitHub Actions Inactive
@ianna ianna marked this pull request as ready for review October 26, 2023 09:39
Copy link
Collaborator Author

@ianna ianna left a comment

Choose a reason for hiding this comment

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

@jpivarski - I see that the lambda functions have different identity:

{(<ufunc 'add'>, 'xyr', 'xyr'): <function test_ArrayBuilder_non_picklable_behavior.<locals>.make_add_xyr.<locals>.add_xyr at 0x12395de40>}
{(<ufunc 'add'>, 'xyr', 'xyr'): <function test_ArrayBuilder_non_picklable_behavior.<locals>.make_add_xyr.<locals>.add_xyr at 0x1242d25c0>}

@agoose77
Copy link
Collaborator

I've loosened the test to just ensure that the .keys() agree. The other test ensures that the behavior with non-serialised lambdas is equal, which I think is sufficient.

@agoose77 agoose77 merged commit 65dc991 into main Oct 26, 2023
37 checks passed
@agoose77 agoose77 deleted the ianna/support_non-default_behaviours branch October 26, 2023 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Numba's njit doesn't support non-default behaviors
3 participants