Skip to content

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Apr 9, 2020

Supersedes #384

The reasoning behind this is that having accessors which hold a reference back to the Parameterized object create a circular reference which stops CPython's garbage collection from immediately collecting a Parameterized instance when it is no longer referenced by any external object instead having to wait for the next gc.collect() cycle.

@philippjfr philippjfr force-pushed the parameters_property branch from 922882b to ec7e668 Compare April 9, 2020 12:32

@property
def _BATCH_WATCH(self_):
return self_.self_or_cls._parameters_state['BATCH_WATCH']
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To allow .param to be a property any state that was previously stored on Parameters now needs to be stored on the Parameterized itself. I didn't want to rock the boat too much so for now I've simply proxied the state that is now on the Parameterized class or instance using these properties. If we want to refactor more then all references to parameterized.param._BATCH_WATCH, parameterized.param._TRIGGER, parameterized.param._events and parameterized.param._watchers could be updated to refer to the parameterized._parameters_state instead.

@philippjfr philippjfr changed the title Turn .param into a property on instances Turn .param into a property Apr 9, 2020
@coveralls
Copy link

coveralls commented Apr 9, 2020

Coverage Status

Coverage decreased (-0.03%) to 76.237% when pulling f639cff on parameters_property into bbc09c6 on master.

@philippjfr philippjfr force-pushed the parameters_property branch from bf3030f to 6c48d9c Compare April 9, 2020 17:07
@philippjfr philippjfr force-pushed the parameters_property branch from 6c48d9c to f639cff Compare April 9, 2020 17:15
@jlstevens
Copy link
Contributor

Looks good. If pickling is working we'll want to merge ASAP to test!

@philippjfr
Copy link
Member Author

It is indeed, @jbednar should we merge to start testing?

@philippjfr
Copy link
Member Author

I'll merge and tag soon so we can start testing. Can always revert if we find some horrendous regression.

@philippjfr philippjfr merged commit 3e41abf into master Apr 9, 2020
@jbednar jbednar deleted the parameters_property branch April 9, 2020 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants