fix: transient attrs should not be written to parquet#4209
Conversation
Signed-off-by: Iason Krommydas <iason.krom@gmail.com>
ianna
left a comment
There was a problem hiding this comment.
In general we almost certainly do not want to save transient attributes to Parquet.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
|
|
Can we manually start the integration test when this is merged? |
|
The integration tests did not catch this. A user did |
Head branch was pushed to by a user without write access
|
I see. Can we still run the integration test? I'll make a release from a commit that passes the nightly tests. |
|
Claude noticed that |
|
I plan to make a follow-up which has nothing to do with the fix to enable attrs writing when one uses |
@TaiSakuma I did not notice this and yes the answer is we can. A CI can always be started manually. |
|
I mean I don't really care. The release can be tomorrow. #4210 is also ready and just needs a rebase after this one is merged. So if you want to put it in too that's cool. The bottleneck is the GPU CI here. |
|
The documentation preview is ready to be viewed at http://preview.awkward-array.org.s3-website.us-east-1.amazonaws.com/PR4209 |
|
https://github.com/scikit-hep/integration-tests/actions/runs/29772491297 @TaiSakuma so either review and merge the other PR or don't. I'm fine either way. It just fits adding it now as it deals with parquet attrs. |
|
I confirmed this passed the nightly integration test and the two nightly property tests (all manually started). |
#4079 introduced attrs serialization to parquet. However, transient attrs should not be serialized. Users are seeing errors writing root files loaded with coffea to disk. This fails with awkward 2.10.0
with
A patch release is needed ASAP as users cannot write to parquet in all analysis frameworks if they use awkward 2.10.0.