snps icon indicating copy to clipboard operation
snps copied to clipboard

remove singleton metaclass

Open afaulconbridge opened this issue 4 years ago • 3 comments

The use of a metaclass to make Resource a singleton is a bit counter-intuitive. Particularly as users won't often interact with this class directly, and I've been caught out by passing a value into SNPs(resource_dir='something') and not seeing the expected result because a Resource single had been created elsewhere.

This PR removes that singleton metaclass, and achieves the same behaviour by default by creating the instance as the default argument to the SNPs.__init__ method. Default method arguments are evaluated when the class is defined, therefore objects created as a default argument are singleton.

Users who want to manage the singleton themselves can use the resources_obj argument to pass the Resource instance to use. This also opens the possibility for having different sub-classes e.g. for storage backends such as a database, or AWS S3.

afaulconbridge avatar Nov 26 '20 11:11 afaulconbridge

Codecov Report

Merging #116 (dde6a29) into develop (1989b81) will decrease coverage by 0.62%. The diff coverage is 42.85%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #116      +/-   ##
===========================================
- Coverage    94.48%   93.86%   -0.63%     
===========================================
  Files            8        8              
  Lines         1468     1417      -51     
  Branches       252      253       +1     
===========================================
- Hits          1387     1330      -57     
- Misses          43       46       +3     
- Partials        38       41       +3     
Impacted Files Coverage Δ
src/snps/utils.py 88.73% <ø> (-0.88%) :arrow_down:
src/snps/snps.py 94.55% <20.00%> (-0.99%) :arrow_down:
src/snps/resources.py 94.73% <100.00%> (-0.34%) :arrow_down:
src/snps/ensembl.py 65.85% <0.00%> (-4.88%) :arrow_down:
src/snps/io/reader.py 97.87% <0.00%> (-0.02%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1989b81...dde6a29. Read the comment docs.

codecov-io avatar Nov 26 '20 11:11 codecov-io

This is a better example of how kwargs can be used as a default singleton:

from snps import SNPs
from snps.resources import Resources
s1 = SNPs()
s2 = SNPs()
id(s1._resources) == id(s2._resources)
# True on this branch
# True on master

However, on this branch this would then break down if it is manually specified :

s3 = SNPs(resources_dir="/tmp/resources")
s4 = SNPs(resources_dir="/tmp/resources")
id(s3._resources) == id(s4._resources)
# False on this branch
# True on master

Also note that this is what happens on master, which is the potentially confusing side-effect:

s5 = SNPs(resources_dir="/tmp/foo/resources")
s6 = SNPs(resources_dir="/tmp/bar/resources")
id(s5._resources) == id(s6._resources)
# True and will use /tmp/foo/resources for both

I like the idea of allow a filename or a url string, but I'm not sure that will cover all cases so falling back to a non-string object instead might be necessary.

I'm happy with a single "resources" parameter instead of separating parameters by type. It does break compatibility for some existing users, but that's probably a good thing to make sure everyone is aware of the change in meaning.

afaulconbridge avatar Dec 09 '20 15:12 afaulconbridge

Thanks for those examples, they help explain the issue. How about this?

  1. remove the singleton to remove confusion
  2. keep resources_dir="resources" for backwards compatibility (i.e., resources on the local filesystem)
  3. for new resources, add kwargs as necessary (e.g., resources_url="")

Then a regular Resources object will be instantiated based on those parameters, with preference for what functionality to use based on whether or not particular fields are specified (e.g., if resources_url is specified, use that resource).

apriha avatar Dec 14 '20 02:12 apriha