DM-16957: Add pickle support to DataId class

[andy-slac]

No description provided.

[TallJimbo]

It took me a while to convince myself this was correct, and that worries me a bit, but if it works for you, I'm okay with it.

Maybe it would be best to just add a comment noting (assuming this is actually right) that we just need to implement __getnewargs_ex__ in order to make __new__ run without error, because the automatic attribute-setting logic in unpickling replaces all of the state __new__ is usually responsible for initializing.

[andy-slac]

I'll add that as a comment. Related question may be whether do we need that non-trivial __new__ or can we do it all in __init__?

[andy-slac]

Just to add - I had to write a unit test for that to convince myself that it works as expected 🙂 Pickle stuff is not on my favorite tools list, I'd prefer to avoid specializing that if I could.

[TallJimbo]

The reason I implemented __new__ is to shortcut the common+trivial case where we already have DataId and don't need a new one, by returning the given instance instead of a new one. I imagine we could avoid that by always going through a different named factory function.

[andy-slac]

Dumb question - is it done for optimization only or does the code actually relies on identity of DataIds?

[TallJimbo]

It should be optimization only (I wasn't sure it would be when I was originally writing this code, but I think it is now).