fix: array attrs not being validated at creation and being of inconsistent type

[ikrommyd]

We have been allowing this this whole time:

In [1]: import awkward as ak

In [2]: attrs = {1: 2}

In [3]: ak.Array([1,2,3], attrs=attrs)
Out[3]: <Array [1, 2, 3] type='3 * int64'>

In [4]: x = ak.Array([1,2,3], attrs=attrs)

In [5]: x.attrs
Out[5]: {1: 2}

In [6]: x._attrs
Out[6]: {1: 2}

which is not correct because A) attrs should normally be of type Attrs and not dict and B) attrs should have only string keys if you look at the Attrs class:

awkward/src/awkward/_attrs.py

Lines 47 to 67 in 824329f

	class Attrs(Mapping):
	def __init__(self, data: Mapping[str, Any]):
	self._data = {_enforce_str_key(k): v for k, v in data.items()}
	def __getitem__(self, key: str):
	return self._data[key]
	def __setitem__(self, key: str, value: Any):
	self._data[_enforce_str_key(key)] = value
	def __iter__(self):
	return iter(self._data)
	def __len__(self):
	return len(self._data)
	def __repr__(self):
	return f"Attrs({self._data!r})"
	def to_dict(self):
	return dict(self._data)

Also setting attrs to itself would raise an error

In [7]: x.attrs = attrs
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[7], line 1
----> 1 x.attrs = attrs

File ~/Dropbox/work/pyhep_dev/awkward/src/awkward/highlevel.py:1327, in Array.__setattr__(self, name, value)
   1303 """
   1304 Args:
   1305     where (str): Attribute name to set
   (...)   1324 to add or modify a field.
   1325 """
   1326 if name.startswith("_") or hasattr(type(self), name):
-> 1327     super().__setattr__(name, value)
   1328 elif name in self._layout.fields:
   1329     raise AttributeError(
   1330         "fields cannot be set as attributes. use #__setitem__ or #ak.with_field"
   1331     )

File ~/Dropbox/work/pyhep_dev/awkward/src/awkward/highlevel.py:403, in Array.attrs(self, value)
    400 @attrs.setter
    401 def attrs(self, value: Mapping[str, Any]):
    402     if isinstance(value, Mapping):
--> 403         self._attrs = Attrs(value)
    404     else:
    405         raise TypeError("attrs must be a 'Attrs' mapping")

File ~/Dropbox/work/pyhep_dev/awkward/src/awkward/_attrs.py:49, in Attrs.__init__(self, data)
     48 def __init__(self, data: Mapping[str, Any]):
---> 49     self._data = {_enforce_str_key(k): v for k, v in data.items()}

File ~/Dropbox/work/pyhep_dev/awkward/src/awkward/_attrs.py:72, in _enforce_str_key(key)
     70 def _enforce_str_key(key: Any) -> str:
     71     if not isinstance(key, str):
---> 72         raise TypeError(f"'attrs' keys must be strings, got: {key!r}")
     73     return key

TypeError: 'attrs' keys must be strings, got: 1

We make attrs get validated on array/record/builder creation as they should and also make them appear of consistent type Attrs to the user. Not sometimes dict and sometimes Attrs.

This was found through coffea while noticing that when you open up a file in coffea, attrs are a dict in virtual/eager mode but of type Attrs in dask mode.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.29%. Comparing base (390cbb8) to head (9b975a0).

Additional details and impacted files

Files with missing lines	Coverage Δ
src/awkward/highlevel.py	80.94% <100.00%> (+0.79%)	⬆️

[github-actions]

The documentation preview is ready to be viewed at http://preview.awkward-array.org.s3-website.us-east-1.amazonaws.com/PR3996

[pfackeldey]

Thanks, good catch!

Could you add your example as a test so we're sure to not alter this behavior in the future?

[ianna]

We have been allowing this this whole time:

In [1]: import awkward as ak

In [2]: attrs = {1: 2}

In [3]: ak.Array([1,2,3], attrs=attrs)
Out[3]: <Array [1, 2, 3] type='3 * int64'>

In [4]: x = ak.Array([1,2,3], attrs=attrs)

In [5]: x.attrs
Out[5]: {1: 2}

In [6]: x._attrs
Out[6]: {1: 2}

also:

>>> attrs[1]
2
>>> attrs[1]=3
>>> array.attrs
{1: 3}

[ikrommyd]

Yeah, the Attrs class blocks non-string keys on __setitem__ so that's fixed too.

[ikrommyd]

Thanks, good catch!

Could you add your example as a test so we're sure to not alter this behavior in the future?

Tests added.

[ianna]

@pfackeldey -- could you please add some description to your review for the benefit of other admins? Thanks.

[pfackeldey]

In my initial review (#3996 (review)) I was happy with this change, except for missing tests. Now that you've (@ikrommyd) added them, I'm happy to approve.

[ianna]

In my initial review (#3996 (review)) I was happy with this change, except for missing tests. Now that you've (@ikrommyd) added them, I'm happy to approve.

To summarize: It was a "good catch" and now it has some test :-) . Thanks.