{"action":"created","issue":{"url":"https://api.github.com/repos/hammerlab/pyensembl/issues/114","labels_url":"https://api.github.com/repos/hammerlab/pyensembl/issues/114/labels{/name}","comments_url":"https://api.github.com/repos/hammerlab/pyensembl/issues/114/comments","events_url":"https://api.github.com/repos/hammerlab/pyensembl/issues/114/events","html_url":"https://github.com/hammerlab/pyensembl/pull/114","id":101106876,"number":114,"title":"Refactoring to support species in EnsemblRelease and to simplify download/install logic","user":{"login":"iskandr","id":48441,"avatar_url":"https://avatars.githubusercontent.com/u/48441?v=3","gravatar_id":"","url":"https://api.github.com/users/iskandr","html_url":"https://github.com/iskandr","followers_url":"https://api.github.com/users/iskandr/followers","following_url":"https://api.github.com/users/iskandr/following{/other_user}","gists_url":"https://api.github.com/users/iskandr/gists{/gist_id}","starred_url":"https://api.github.com/users/iskandr/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/iskandr/subscriptions","organizations_url":"https://api.github.com/users/iskandr/orgs","repos_url":"https://api.github.com/users/iskandr/repos","events_url":"https://api.github.com/users/iskandr/events{/privacy}","received_events_url":"https://api.github.com/users/iskandr/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":3,"created_at":"2015-08-14T21:35:31Z","updated_at":"2015-08-19T19:58:12Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/hammerlab/pyensembl/pulls/114","html_url":"https://github.com/hammerlab/pyensembl/pull/114","diff_url":"https://github.com/hammerlab/pyensembl/pull/114.diff","patch_url":"https://github.com/hammerlab/pyensembl/pull/114.patch"},"body":"* Added `annotation_name` and `annotation_version` properties to `Genome`, which are used (along with `reference_name`) to create different directories for each genome annotation database.\r\n* Renamed `compute_cache` to `memory_cache`, require objects that use it to explicitly create a `MemoryCache` instance (rather than relying on global state). \r\n* Hiding the ugliness of direct interactions with `datacache` behind a `DownloadCache` class\r\n* * Got rid of `GenomeSource`: if a file needs to be downloaded, then it's down directly by `Genome` via its `download_cache` instance. \r\n* All wrappers around data (GTF, SequenceData, Database objects) expect their data to already be locally present (and thus can just take paths instead of `GenomeSource` objects)\r\n* Moved `install_string` onto Genome, got rid of `install_string_python`.\r\n* Added `Species` objects, which track names, synonyms, and the Ensembl releases associated with particular reference assemblies for each species.\n\n<!-- Reviewable:start -->\n[<img src=\"https://reviewable.io/review_button.png\" height=40 alt=\"Review on Reviewable\"/>](https://reviewable.io/reviews/hammerlab/pyensembl/114)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/hammerlab/pyensembl/issues/comments/132760478","html_url":"https://github.com/hammerlab/pyensembl/pull/114#issuecomment-132760478","issue_url":"https://api.github.com/repos/hammerlab/pyensembl/issues/114","id":132760478,"user":{"login":"iskandr","id":48441,"avatar_url":"https://avatars.githubusercontent.com/u/48441?v=3","gravatar_id":"","url":"https://api.github.com/users/iskandr","html_url":"https://github.com/iskandr","followers_url":"https://api.github.com/users/iskandr/followers","following_url":"https://api.github.com/users/iskandr/following{/other_user}","gists_url":"https://api.github.com/users/iskandr/gists{/gist_id}","starred_url":"https://api.github.com/users/iskandr/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/iskandr/subscriptions","organizations_url":"https://api.github.com/users/iskandr/orgs","repos_url":"https://api.github.com/users/iskandr/repos","events_url":"https://api.github.com/users/iskandr/events{/privacy}","received_events_url":"https://api.github.com/users/iskandr/received_events","type":"User","site_admin":false},"created_at":"2015-08-19T19:58:12Z","updated_at":"2015-08-19T19:58:12Z","body":"\n\n\n\nReview status: all files reviewed at latest revision, 33 unresolved discussions, all commit checks successful.\n\n---\n\n<sup>**[pyensembl/database.py, line 70 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5TzS-Qo-OsSFuJbAe)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/database.py#L70)):</sup>\nI'm thinking of the GTF object as an in-memory representation of a GTF file. Why wouldn't they be equal to each other? \n\n---\n\n<sup>**[pyensembl/database.py, line 260 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5UMV2jYGV3BGeA0WQ)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/database.py#L260)):</sup>\nI think that instead I'm just shifting the responsibility for making sure stuff exists to `Genome` and simplifying the amount of logic allowed in `Database`. \n\nThat fits with the general trend of this PR of centralizing anything tricky and having the \"leaf\" objects be dumber and do less. \n\n---\n\n<sup>**[pyensembl/download_cache.py, line 70 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5IYIUY1uhplxqWYMG)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/download_cache.py#L70)):</sup>\nIt was at one point :-)\n\n---\n\n<sup>**[pyensembl/download_cache.py, line 86 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5KPgMlhRo5KPTtlMW)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/download_cache.py#L86)):</sup>\nI think it's actually better not to do this by default (since it eliminates the need to \"install\" fully local genomes), but yeah we can just always stick with one behavior. \n\n---\n\n<sup>**[pyensembl/download_cache.py, line 246 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5No4duFXSQeZIUnMw)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/download_cache.py#L246)):</sup>\n`MissingLocalFile` is a legit exception that the user needs to take care of (i.e. the file they think exists locally doesn't)\n\n---\n\n<sup>**[pyensembl/download_cache.py, line 249 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5Ls2lKcjUd0AiJOrN)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/download_cache.py#L249)):</sup>\nYeah, this is the old version hanging around, got rid of it. \n\n---\n\n<sup>**[pyensembl/download_cache.py, line 280 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5LyMiPyLi1VW2n8Jv)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/download_cache.py#L280)):</sup>\nI changed them to delete_cached_files and delete_cache_directory\n\n---\n\n<sup>**[pyensembl/ensembl_release.py, line 42 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5Z2GdoSwCfB-8Q2a0)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/ensembl_release.py#L42)):</sup>\nMaybe whenever we have more direct support for RefSeq? The current \"generality\" of PyEnsembl still seems preparatory/aspirational. \n\n---\n\n<sup>**[pyensembl/ensembl_release.py, line 80 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5J0yO0eKnAMoajyFr)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/ensembl_release.py#L80)):</sup>\nChanged to `missing_urls_dict`. \n\n---\n\n<sup>**[pyensembl/ensembl_release.py, line 81 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5ZZoO4wxM6Z6y5jMP)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/ensembl_release.py#L81)):</sup>\nIt's still possible but since these insides are shifting, let's not expose it to the user and instead give them only One Way To Do It. \n\n---\n\n<sup>**[pyensembl/ensembl_release.py, line 90 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5_6vg5VhTF8MszeNN)** ([raw file](https://github.com/hammerlab/pyensembl/blob/d9e4f1cf44667d876d0513b2d1a937fd11675f95/pyensembl/ensembl_release.py#L90)):</sup>\nWe don't need it anymore, since releases are isolated within their own directories. \n\n---\n\n<sup>**[pyensembl/genome.py, line 181 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx1D3O3C-GXG0bRp6wj)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/genome.py#L181)):</sup>\nAny use of subdirectories is because we're relying on datacache to figure out the global path for us, which is something we need to move away from to then cut the dependency on datacache (or make datacache smaller and more focused). \n\n---\n\n<sup>**[pyensembl/genome.py, line 184 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5ShQsMtNnx9QIG2Z-)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/genome.py#L184)):</sup>\nBecause it loads all the data into memory *and* if auto_download is True then it installs whatever is missing. That `install` from the shell is implemented using this function is an implementation detail. \n\n---\n\n<sup>**[pyensembl/genome.py, line 212 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5a8pxz7jpSvk-qszH)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/genome.py#L212)):</sup>\nI tried for a while to do all the data loading up front but couldn't find an easy way to make it work with constructing `EnsemblRelease` objects but not immediately downloading/indexing their data (which you want to do to e.g. clear their cache directory). \n\n---\n\n<sup>**[pyensembl/genome.py, line 231 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5OrOki0nIG_9cR6Iu)** ([raw file](https://github.com/hammerlab/pyensembl/blob/d9e4f1cf44667d876d0513b2d1a937fd11675f95/pyensembl/genome.py#L231)):</sup>\n`Genome(..., auto_download=True).load_all_data()`. But, there shouldn't be much reason to do this outside the shell script. \n\nI don't think there's any reason to expose indexing to the user since all I ever find myself wanting to do with index files is *delete* them because they're outdated in some way. \n\nThe tests don't need to call `load_all_data` because they either depend on having a large number of Ensembl releases installed or use local files which get used in-place.\n\n---\n\n<sup>**[pyensembl/genome.py, line 239 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5OR3RQjpTZ_F_1TDj)** ([raw file](https://github.com/hammerlab/pyensembl/blob/d9e4f1cf44667d876d0513b2d1a937fd11675f95/pyensembl/genome.py#L239)):</sup>\nI do, you can achieve the same effect with \n\n```\npyensembl delete-sequence-cache --release 75\npyensembl install --release 75 \n```\n\nWe can add a command to e.g. delete-cache or delete-gtf-cache whenever we need those cases. \n\n---\n\n<sup>**[pyensembl/genome.py, line 280 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5L6R0e6lZyW327B-j)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/genome.py#L280)):</sup>\nThis is really a hack, I'll more comments explaining what's going on. But, we download fasta and gtf files (and I guess their gzipped versions) and only create pickle and db files as temporaries. I switched this code to at least delete the expected \"temporary\" filetypes rather than delete everything other than FASTA/GTF files. \n\n---\n\n<sup>**[pyensembl/gtf.py, line 40 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5PJ6_GfpT_hOYpPQB)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/gtf.py#L40)):</sup>\nYeah, I wasn't totally thrilled with having path munging this deep into the object graph but I don't know how else to co-locate the GTF temporary files with the other cached data. That said, the way I'm saving .CSV snapshots of the GTF is questionably useful, since I rarely look at GTF subsets as a dataframe (the way I originally envisioned I would want to). So maybe we can eventually get rid of this? Particular if we can speed up GTF parsing so that reading a GTF file twice wouldn't be devastatingly slow. \n\n---\n\n<sup>**[pyensembl/sequence_data.py, line 32 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5QfowjWA37FTrSvoN)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/sequence_data.py#L32)):</sup>\nOn the other hand, these objects are more independent and testable. A SequenceData object taking a FASTA path directly and having a free parameter for where you tell it to store derived/cached files seems fine to me. \n\n---\n\n<sup>**[pyensembl/shell.py, line 145 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx1Jve64C8HnL1y7XI9)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/shell.py#L145)):</sup>\nI'm removing the default docstring since I'd rather have the user do stuff more explicitly. \n\n---\n\n<sup>**[pyensembl/shell.py, line 153 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx1KwRN8jyP_1zvazOG)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/shell.py#L153)):</sup>\nJust `delete` also nukes the (large) downloaded GTF and FASTA files. I'd rather add more fine-grained delete options as we need them. \n\n---\n\n<sup>**[pyensembl/species.py, line 128 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx1ON6Midu_ZnB0Quoo)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/species.py#L128)):</sup>\nI'm not happy about this either but it's the only easy way to get mice working for now. I wanted to get the species representation wired together first, maybe we can do a later PR to auto-populate from remote data?\n\n---\n\n<sup>**[pyensembl/species.py, line 135 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx1NyrgRIEh78g2RpVJ)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/species.py#L135)):</sup>\nI'd rather species = \"human\" or \"mouse\" than \"mus_musculus\". \n\n---\n\n<sup>**[pyensembl/species.py, line 136 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx1M9EXqfBCqfrnrE2A)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/pyensembl/species.py#L136)):</sup>\n?\n\n---\n\n<sup>**[pyensembl/transcript.py, line 388 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx1Ocsj9kPNBmkN7V9u)** ([raw file](https://github.com/hammerlab/pyensembl/blob/d9e4f1cf44667d876d0513b2d1a937fd11675f95/pyensembl/transcript.py#L388)):</sup>\n`transcript_sequences` is a property which can raise its own errors (and thus doesn't need to be checked externally)\n\n---\n\n<sup>**[pyensembl/transcript.py, line 465 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5T7s8g2u2p1A0o3zI)** ([raw file](https://github.com/hammerlab/pyensembl/blob/d9e4f1cf44667d876d0513b2d1a937fd11675f95/pyensembl/transcript.py#L465)):</sup>\n^ same as above\n\n---\n\n<sup>**[setup.py, line 68 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5RtOSRdTK34hPEs76)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/setup.py#L68)):</sup>\nNope, removing\n\n---\n\n<sup>**[test/test_download.py, line 0 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5S39pRTiuUDKu6qbF)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/test/test_download.py#L0)):</sup>\nThese mocked out tests can be tedious since they are tied to the internals in a way that's annoying to update when the internals change. I was thinking about fixing this test but we're still due for more changes down the road and I don't think I have the emotional energy to maintain tests that know about internal state. \n\n---\n\n<sup>**[test/test_missing_genome_sources.py, line 55 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx1PeDBkTNMeaNxC5ZC)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/test/test_missing_genome_sources.py#L55)):</sup>\nJust a missing copy/paste, the directory name doesn't really matter though.\n\n---\n\n<sup>**[test/test_mouse.py, line 34 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx1Pk68Iqc1avDPYFFe)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/test/test_mouse.py#L34)):</sup>\nSure, but I don't think it actually matters which directories we use for small test data. We'll end up with some new tests that maybe use a different directory and I think that's probably fine.\n\n---\n\n<sup>**[test/test_sequence_data.py, line 34 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5TOnvc42RN9Ei60pu)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/test/test_sequence_data.py#L34)):</sup>\nChecking ValueError text seems really future-unfriendly. I'll just move the raises check further in. \n\n---\n\n<sup>**[test/test_ucsc_gtf.py, line 60 \\[r1\\]](https://reviewable.io:443/reviews/hammerlab/pyensembl/114#-Jx5TdntweCBjnQcibyd)** ([raw file](https://github.com/hammerlab/pyensembl/blob/a97a7cc2225e180fe67e46f272cb1fab199163fd/test/test_ucsc_gtf.py#L60)):</sup>\n(1) It's a local path, so the files don't need to be copied to the cache\n\n(2) `Genome.genes()` uses the `Genome.db` property which triggers lazy loading of the GTF and construction of the associated database. \n\n---\n\n\nComments from the [review on Reviewable.io](https://reviewable.io:443/reviews/hammerlab/pyensembl/114)\n<!-- Sent from Reviewable.io -->\n"}}