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: return proxy of attrs, behavior; dak.from_awkward correctly sets attrs #448

Merged
merged 9 commits into from
Jan 29, 2024

Conversation

agoose77
Copy link
Collaborator

Closes #447 by making .attrs and .behavior read-only. This addresses a confusing part of the API: currently these are writeable, but the underlying compute does not share the results of mutating attrs or behavior.

If we need to, we can add a dask_awkward.with_attrs to add a mechanism of mutating these objects. I'm not sure what the ramifications of this PR will be — we may internally be expecting to pass around the true attrs dicts, which would be broken by this change.

@lgray
Copy link
Collaborator

lgray commented Jan 11, 2024

So I can only add attrs to an awkward array and then make a dask-awkward array from that?

This probably breaks nanoevents?

@lgray
Copy link
Collaborator

lgray commented Jan 11, 2024

yep:

self = <coffea.nanoevents.factory.NanoEventsFactory object at 0x12a0f1330>

    def events(self):
        """Build events"""
        if self._is_dask:
            events = self._mapping(form_mapping=self._schema)
            report = None
            if isinstance(events, tuple):
                events, report = events
>           events.attrs["@original_array"] = events
E           TypeError: 'mappingproxy' object does not support item assignment

@agoose77
Copy link
Collaborator Author

You can still override attrs of a dask_awkward array, but it's harder to do. You just copy the meta and patch the .attrs. The intention of this PR is that the user-facing .attrs should not be writeable.

@lgray
Copy link
Collaborator

lgray commented Jan 11, 2024

Ah, indeed I can see a few work arounds. I'll confirm one in an hour or so.

@lgray
Copy link
Collaborator

lgray commented Jan 12, 2024

Yep - all good - simple work around found.

lgray added a commit to CoffeaTeam/coffea that referenced this pull request Jan 12, 2024
@douglasdavis
Copy link
Collaborator

based on conversation this morning it seemed that this is good to go?

@martindurant
Copy link
Collaborator

(test coverage would be nice)

@lgray
Copy link
Collaborator

lgray commented Jan 21, 2024

@douglasdavis yes this is fine from my end

@lgray lgray force-pushed the agoose77/fix-attrs-readonly branch from cfd9289 to c2d5c43 Compare January 27, 2024 14:19
@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (782b831) 93.10% compared to head (bd52045) 93.17%.
Report is 1 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #448      +/-   ##
==========================================
+ Coverage   93.10%   93.17%   +0.07%     
==========================================
  Files          23       23              
  Lines        3293     3298       +5     
==========================================
+ Hits         3066     3073       +7     
+ Misses        227      225       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lgray
Copy link
Collaborator

lgray commented Jan 27, 2024

@martindurant let me know if this is sufficient for tests, otherwise I think this one is good to go. Thanks in advance for the review.

@lgray lgray changed the title fix: return proxy of attrs, behavior fix: return proxy of attrs, behavior; dak.from_awkward correctly sets attrs Jan 27, 2024
@lgray
Copy link
Collaborator

lgray commented Jan 27, 2024

@martindurant fixing a few things has led to scope creep of this PR... 🤷 Anyway it's ready for another look.

@martindurant martindurant merged commit d2249d4 into main Jan 29, 2024
25 checks passed
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.

bug: attrs do not survive compute()
5 participants