pickle setstate: setattr __dict__ only if not empty

[rwgk]

Description

The motivation for this PR is to not force the use of py::dynamic_attr() unnecessarily. Concrete use case:

https://github.com/deepmind/open_spiel/blob/643dfd2a5ef023e796e20d06758800ec34a1204b/open_spiel/python/pybind11/pyspiel.cc#L309

py::dynamic_attr() comes with the disadvantage that it enables adding arbitrary attributes, which opens the door to accidents that tend to be subtle and time-consuming to debug (example situation below). The cost for avoiding this disadvantage is minuscule: just one additional if with 2 simple conditions in pybind11/detail/init.h.

The newly added unit tests reflect a typical situation in which this PR helps: a wrapped C++ virtual base class with a trampoline, combined with the use of base class pointers to C++ derived classes.

Example situation:

• You need to pickle a C++ derived class (as in the new test), but without this PR that is only possible if you use py::dynamic_attr().
• Therefore you decide to add py::dynamic_attr().
• Time goes by ...
• Someone accidentally assigns a misspelled attribute, but because of py::dynamic_attr() the code runs and produces nonsensical results. It takes you N hours to trace that out.

Suggested changelog entry:

Pickle support enhancement: ``setstate`` implementation will attempt to ``setattr`` ``__dict__`` only
if the unpickled ``dict`` object is not empty, to not force use of ``py::dynamic_attr()`` unnecessarily.

[EricCousineau-TRI]

Sweet!

At high-level, motivation not immediately obvious for me - was it speed, or correctness, and can you mention publicly visible stakeholder (Google) or proj name (deepspiel open_spiel or what not)?

[EricCousineau-TRI]

And for given motivation, is it possible to give precise numbers (or rough) in terms of performance gain (if it was speed / memory)?

[rwgk]

And for given motivation, is it possible to give precise numbers (or rough) in terms of performance gain (if it was speed / memory)?

Performance isn't the main concerns, but py::dynamic_attr() enables adding arbitrary attributes, which opens the door to accidents (often subtle and therefore difficult/time-consuming to debug).

Yes, this is for DeepMinds open_spiel: https://github.com/deepmind/open_spiel
This will become public later. Currently it's still an internal pending change. Once submitted it'll get pushed out on some weekly schedule, therefore nothing concrete to point to yet.

Thanks for looking Eric, I'll go ahead working on the unit test and will add the motivation to the description.

[rwgk]

Hi @EricCousineau-TRI and @elkhrt, this PR is complete now. Could you please review?

[rwgk]

The one CI failure is just our most common flake (see #2995).

[elkhrt]

Thanks Ralf - this is great.

[EricCousineau-TRI]

Looks good, thank you for providing context (and soz for delay!)

Requesting more concrete testing, with nits on consistent styling w/ existing and usage of more explicitly conditionals in lieu of try/catch. (Feel free to punt on nits!)

For the overview, I can see two possible "debugging" issue:
(1) you accidentally assign a non-existent attribute but py::dynamic_attr() allows it (thus "ouch", you gotta trace that out)
(2) is that you cannot unpickle a C++ derived class, motivating py::dynamic_attr(), forcing you to run into (1).

Is it possible for you to rewrite the overview / commit stmt to expliclty focus on (2)?
(then (1) becomes an obvious side effect?)

Lemme know if you have any Q's or would like push back on this!

[rwgk]

Thanks Eric! I updated the code and the PR description starting from your suggestion.

[rwgk]

Apparently the Centos 8 build broke for external reasons on 6/3. Trying again (PR Close-Reopen) to see if it fixed itself.

[EricCousineau-TRI]

Looks great! However, I think you possibly uncovered a new bug 😿
I don't think we need to block this PR on it, but we should at least leave paper trail (issue+TODO) for it!

[rwgk]

Closing-Reopening to trigger CI.