Introduce picklable configurables

[gmrukwa]

What is the subject of this PR?

I was trying to implement a mechanism that allows the external configurables and scoped configurables get pickled.

Resolves #31

Details

Since the only thing that we need is the decorated __new__ or __init__, I thought that we could replace the instance that is actually returned to the instance of original class. This instance gets initialized with the decorated function. Through this we get:

• picklability of all configurable versions of picklable types (regardless it is scoped, external or plain configurable)
• more natural and intuitive inheritance: isinstance(subclass_instance, type(superclass_instance)) evaluates to True even if superclass is an external configurable

Remarks

✅ I signed the CLA.

[gmrukwa]

Hey @dhr , @sguada, could you please provide some feedback on this PR? If there's something I need to change before this gets merged, I would need to know. For now I have a workaround in my own code (thanks Python!), but I wanted to share this with others, as the issue seems quite frequent.

[gmrukwa]

Hello @dhr, @sguada! Any progress?

[gmrukwa]

Hello, is there anyone capable of reviewing?

[pseeth]

I also just ran into this problem. A fix would be nice. Right now, the only thing I can do is wrap everything in functions to try and fix it so they get evaluated within the script and return the original class rather than the gin class.

[gmrukwa]

@pseeth, as you see I am waiting for more than a month for the review. 😞

[gmrukwa]

@pseeth you can make yourself a module (like this one I did) and make sure it gets imported, so the gin works properly.

[pseeth]

That looks nice! Thanks! I ended up writing an "unginifiy" configurable function which looks for the same class using a globals() hack and then finds the associated arguments in the gin-config and reinstantiates the class: https://github.com/nussl/models/blob/master/src/gin_utils/__init__.py#L36

[gmrukwa]

@dhr, @sguada, any progress?

[sguada]

@gmrukwa sorry @dhr has found some issues this current solution for metaclasses. He will be adding more details here

[gmrukwa]

@gmrukwa sorry @dhr has found some issues this current solution for metaclasses. He will be adding more details here

I'd love to discuss it further 🙂 If I can do anything more to keep things going, just let me know.

[gmrukwa]

Basically this version makes inheritance of a nested class fully transparent (like nonexistent), just influencing the parameters for object creation.

[dhr]

Thanks for the change! And very sorry I didn't take a closer look at this until recently. After testing internally, there are a couple of issues as detailed more thoroughly in the comment. Let me know if you have any thoughts on how to address those.

[google-cla]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

[google-cla]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

[wprazuch]

@googlebot I fixed it.

[google-cla]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[wprazuch]

@googlebot I consent.

[gmrukwa]

@sguada, @dhr, I fixed the issues you explained with the __reduce__ approach 🙂 Perhaps we could merge it now, when you review it and I polish the details according to your suggestions. I've also added two tests that I skip by default, but it should be considered what should happen in these scenarios. I believe these are replicating what happens in Sonnet, though the behavior of the current code version against these tests is exactly the same as with the original code.

[dhr]

Thanks! This all looks really good to me. And special thanks for the thorough test additions. I haven't had a chance to try it all out internally yet, will do that tomorrow and if that looks good will approve and merge it in.

[dhr]

I wonder if we could remove the "_gin_decorated" attribute and associated base class traversal above if we use __class__ = fn_or_cls.__class__ here? I think that would propagate the right __class__ down even when decorated multiple times (although haven't tested).

[gmrukwa]

I've tested that (or at least what I think you think of) and it didn't work so smoothly for external configurables (the case when you have scopes, etc.) 🙂

[gmrukwa]

@dhr, @sguada, can we merge it now that all conflicts got resolved?

[gmrukwa]

@dhr, @sguada any news? :)

[dhr]

Hey, sorry for the delay again. I think we're close to resolving everything... Unfortunately the approach with __reduce__ and overriding __class__ ran into a bunch of incompatibilities as well, involving some unforeseen errors raised by an internal library used in some projects.

Instead of changing that library (may be possible, but would take longer) I've prototyped a solution using wrapt to proxy the classes instead. There have been quirks with that approach as well, but I think it's very close to working (still resolving one or two remaining issues). Once that's good to go/checked in (hopefully today), I'll merge most of your code here (in particular the added tests, which have been very helpful).

Thanks again for all the contributions and sorry for the slow turnaround!

[dhr]

Further update... The wrapt approach also didn't pan out in the end, due to some fast-path type checks in NumPy (using lower-level CPython APIs) that don't play well with proxies. However, I had a new idea that finally seems to actually work and is backwards compatible with all internal projects. I think it's also the most principled/cleanest solution in the end. That's over in #100. Once that's checked in I'll finally merge the tests here.

[valentinkoe]

#100 has been merged but reverted again in 9853120 . Are there any plans on how to proceed with this?

[dhr]

This should finally all be resolved. There were some tricky to debug issues with the original commit #100 that required a rollback (in particular in the end it turned out there were some internal users who were dynamically subclassing registered/external configurable classes who were broken by the change). I finally had time to reinvestigate and fix things up... a roll-forward with those fixes just landed in #116, and the tests from this pull request were merged in 26d134e. Closing this PR now, sorry for the long delay!

[gmrukwa]

Thanks, @dhr! It took a long time but was a pleasure to collaborate 🙂