Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deserialization should construct cached EnsemblRelease objects #163

Merged
merged 5 commits into from
Sep 14, 2016

Conversation

iskandr
Copy link
Contributor

@iskandr iskandr commented Sep 14, 2016

  • Moved contents of cached_release onto the EnsemblRelease object (EnsemblRelease.cached)
  • Using WeakValueDictionary instead of ordinary dict to avoid memory leak
  • Added from_dict class method to use cached for deserialization

This change is Reviewable

@iskandr
Copy link
Contributor Author

iskandr commented Sep 14, 2016

It would be nice to do something similar for the Genome base class but I would wait until we have a more automatic way of adding this functionality to a class (maybe via a metaclass or modification to the Serializable base class?)

the same EnsemblRelease object since each one will store a lot of cached
annotation data in-memory.
Keeping this function for backwards compatibility but this functionality
has been moving onto the EnsemblRelease object itself by overriding __new__.
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are you overriding new?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, that's left over from an old attempt (turned out too arcane).

if init_args_tuple in cls._genome_cache:
genome = cls._genome_cache[init_args_tuple]
else:
genome = cls._genome_cache[init_args_tuple] = cls(*init_args_tuple)
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't even know this was valid syntax! It's right to left?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, right to left (like Arabic!). It's a generalization of a = b = None.

@tavinathanson
Copy link
Contributor

Agree that we need to generalize this, but good to have a solution for now!

@tavinathanson
Copy link
Contributor

LGTM

@coveralls
Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage increased (+0.07%) to 81.491% when pulling 0fa961d on use-cached-ensembl-release-objects-after-pickling into 423af3c on master.

@iskandr iskandr merged commit 6fbf8ad into master Sep 14, 2016
@coveralls
Copy link

coveralls commented Sep 14, 2016

Coverage Status

Coverage increased (+0.07%) to 81.491% when pulling 0fa961d on use-cached-ensembl-release-objects-after-pickling into 423af3c on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 81.427% when pulling 0fa961d on use-cached-ensembl-release-objects-after-pickling into 423af3c on master.

@iskandr iskandr deleted the use-cached-ensembl-release-objects-after-pickling branch September 15, 2016 23:05
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.

None yet

3 participants