{"action":"created","issue":{"url":"https://api.github.com/repos/ga4gh/server/issues/607","labels_url":"https://api.github.com/repos/ga4gh/server/issues/607/labels{/name}","comments_url":"https://api.github.com/repos/ga4gh/server/issues/607/comments","events_url":"https://api.github.com/repos/ga4gh/server/issues/607/events","html_url":"https://github.com/ga4gh/server/pull/607","id":104811322,"number":607,"title":"G2P '/genotypephenotype/search' endpoint","user":{"login":"bwalsh","id":47808,"avatar_url":"https://avatars.githubusercontent.com/u/47808?v=3","gravatar_id":"","url":"https://api.github.com/users/bwalsh","html_url":"https://github.com/bwalsh","followers_url":"https://api.github.com/users/bwalsh/followers","following_url":"https://api.github.com/users/bwalsh/following{/other_user}","gists_url":"https://api.github.com/users/bwalsh/gists{/gist_id}","starred_url":"https://api.github.com/users/bwalsh/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bwalsh/subscriptions","organizations_url":"https://api.github.com/users/bwalsh/orgs","repos_url":"https://api.github.com/users/bwalsh/repos","events_url":"https://api.github.com/users/bwalsh/events{/privacy}","received_events_url":"https://api.github.com/users/bwalsh/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":6,"created_at":"2015-09-04T01:20:44Z","updated_at":"2015-09-10T11:27:23Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/ga4gh/server/pulls/607","html_url":"https://github.com/ga4gh/server/pull/607","diff_url":"https://github.com/ga4gh/server/pull/607.diff","patch_url":"https://github.com/ga4gh/server/pull/607.patch"},"body":"## Summary\r\nWe extended the GA4GH Reference server to include a the '/genotypephenotype/search' endpoint.  This document describes the work to date  \r\n\r\n## Approach\r\nWe based our work on the model captured in [ga4gh/schemas commit of Jul 30, 2015](https://github.com/ga4gh/schemas/tree/be171b00a5f164836dfd40ea5ae75ea56924d316).  This version of the schema predates the [separated genotype to phenotype files from baseline](https://github.com/ga4gh/schemas/commit/846b711fdcf544bf889cc7dbab19c6c48e9a9428).\r\n\r\nThe code was based on a [branch](https://github.com/ga4gh/server/tree/g2p) setup for this purpose by the server team.\r\nNo major refactoring of the server was needed, additional code was added to ga4gh/backend.py,ga4gh/frontend.py and test/unit/test_views.py.  A new model was created in datamodel/genotype_phenotype.py\r\n\r\n## Data\r\nThe cancer genome database [Clinical Genomics Knowledge Base](http://nif-crawler.neuinfo.org/monarch/ttl/cgd.ttl) published by the Monarch project was the source of Evidence.   \r\n\r\n\r\n![image](https://cloud.githubusercontent.com/assets/47808/9338065/a0a84b8e-4597-11e5-82ed-65d7b9f3ae97.png)\r\n\r\n\r\n## API\r\nThe GA4GH schemas define a single endpoint `/genotypephenotype/search` which accepts a POST of a [request body](https://github.com/ga4gh/schemas/blob/be171b00a5f164836dfd40ea5ae75ea56924d316/src/main/resources/avro/genotypephenotypemethods.avdl#L102)  containing one or more of [Feature](https://github.com/ga4gh/schemas/blob/be171b00a5f164836dfd40ea5ae75ea56924d316/src/main/resources/avro/sequenceAnnotations.avdl#L105), [PhenotypeInstance](https://github.com/ga4gh/schemas/blob/be171b00a5f164836dfd40ea5ae75ea56924d316/src/main/resources/avro/genotypephenotype.avdl#L77), [EnvironmentalContext](https://github.com/ga4gh/schemas/blob/be171b00a5f164836dfd40ea5ae75ea56924d316/src/main/resources/avro/genotypephenotype.avdl#L49), and [Evidence](https://github.com/ga4gh/schemas/blob/be171b00a5f164836dfd40ea5ae75ea56924d316/src/main/resources/avro/genotypephenotype.avdl#L113) which are combined as a logical AND to query the underlying datastore.  Missing types are treated as a `wildcard` returning all data.  Responses of matching data are returned as a list of  [FeaturePhenotypeAssociation](https://github.com/ga4gh/schemas/blob/be171b00a5f164836dfd40ea5ae75ea56924d316/src/main/resources/avro/genotypephenotype.avdl#L132).  All types rely heavily on [OntologyTerm](https://github.com/ga4gh/schemas/blob/be171b00a5f164836dfd40ea5ae75ea56924d316/src/main/resources/avro/ontologies.avdl#L10)\r\n\r\n### Request\r\n![image](https://cloud.githubusercontent.com/assets/47808/9643362/4e081ae0-5176-11e5-8550-abd9c7c43d23.png)\r\nhttp://yuml.me/edit/bf06b90a\r\n\r\n### Response\r\n![image](https://cloud.githubusercontent.com/assets/47808/9339152/53d42aca-459d-11e5-8c91-204f42dc233a.png)\r\n\r\nhttp://yuml.me/edit/25343da1\r\n\r\n----\r\n## Implementation\r\n\r\n![image](https://cloud.githubusercontent.com/assets/47808/9591417/18e1d2c0-4ff0-11e5-90b6-3d2d4f76d31e.png)\r\n\r\nhttp://yuml.me/c97fada2\r\n\r\n\r\n----\r\n## Issues \r\n\r\n### Query by example\r\n\r\nThere are four datatypes types for each entity [string, external identifier, ontology identifier and 'entity'].\r\nCurrently the implementation handles queries of [string, external identifier and ontology identifier].  \r\n\r\nThe  'entity' query is a type of query-by-example has been deferred.  Challenges that arose: \r\n\r\n* schema constraints:  there are several fields within the schemas that are defined as non-null. This may be fine when creating an entity from a data store, however, they are problematic when creating an entity to be used in a query.\r\n\r\n* additional discussions needed to determine what properties from an existing entity will be used for the query and which will be ignored.  For example [a Feature](https://github.com/ga4gh/schemas/blob/6725fb6653eb5642c0dc4884c82016db1c24c5c3/src/main/resources/avro/sequenceAnnotations.avdl#L105) has [id,parentIds, featureSetId, referenceName, start,end, strand, featureType, attributes]  we need to specify exactly what the query's expectations are.  \r\n\r\n*Recommendation:* Leave the schema definitions as-is.  However, leave the entity query-by-example unimplemented.  Implement when demand exists with sufficient use case details.\r\n\r\n \r\n### Name collision (SearchFeaturesResponse)\r\n\r\nThat schema contains two definitions of the class `[SearchFeaturesRequest,SearchFeaturesResponse]`.  These conflict in the generated code with other classes of the same name.\r\n\r\nThe schema project the current server is based on is  `version = '0.6.be171b00'`\r\nSnippets from this commit follow\r\n\r\n* One in the file genotypephenotypemethods.avdl,  protocol GenotypePhenotypeMethods\r\n```\r\n/** This is the response from `POST /genotypephenotype/search` expressed as JSON. */\r\nrecord SearchFeaturesResponse {\r\n  /**\r\n  The list of matching FeaturePhenotypeAssociation.\r\n  */\r\n  array<org.ga4gh.models.FeaturePhenotypeAssociation> associations = [];\r\n\r\n  ...\r\n```\r\n\r\n* The second one is found in sequenceAnnotationmethods.avdl \r\n```\r\n  /** This is the response from `POST /features/search` expressed as JSON. */\r\n  record SearchFeaturesResponse {\r\n    /**\r\n    The list of matching annotations, sorted by start position. Annotations which\r\n    share a start position are returned in a deterministic order.\r\n    */\r\n    array<org.ga4gh.models.Feature> features = [];\r\n\r\n    ... \r\n```\r\n\r\n* The generated code only has the class associated with `sequenceAnnotationmethods.avdl`\r\n \r\n\r\nBoth sequenceAnnotationmethods.avdl and genotypephenotypemethods.avdl share the same namespace `@namespace(\"org.ga4gh.methods\")` each file defines an enclosing `protocol`.\r\n\r\nIn the [names section of the spec](https://avro.apache.org/docs/1.7.7/spec.html#Names)\r\n\r\n```\r\nA name only is specified, i.e., a name that contains no dots. In this case the namespace is taken from the most tightly enclosing schema or protocol. For example, if \"name\": \"X\" is specified, and this occurs within a field of the record definition of org.foo.Y, then the fullname is org.foo.X. If there is no enclosing namespace then the null namespace is used.\r\n```\r\n\r\nThe schemas pass validation.\r\n```\r\nA schema or protocol may not contain multiple definitions of a fullname. Further, a name must be defined before it is used (\"before\" in the depth-first, left-to-right traversal of the JSON parse tree, where the types attribute of a protocol is always deemed to come \"before\" the messages attribute.)\r\n```\r\n\r\n*Recommendation:*  Rename the GenotypePhenotypeMethods [SearchFeaturesRequest,SearchFeaturesResponse] to [SearchGenotypePhenotypeRequest,SearchGenotypePhenotypeResponse]\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/ga4gh/server/issues/comments/139208912","html_url":"https://github.com/ga4gh/server/pull/607#issuecomment-139208912","issue_url":"https://api.github.com/repos/ga4gh/server/issues/607","id":139208912,"user":{"login":"jeromekelleher","id":2664569,"avatar_url":"https://avatars.githubusercontent.com/u/2664569?v=3","gravatar_id":"","url":"https://api.github.com/users/jeromekelleher","html_url":"https://github.com/jeromekelleher","followers_url":"https://api.github.com/users/jeromekelleher/followers","following_url":"https://api.github.com/users/jeromekelleher/following{/other_user}","gists_url":"https://api.github.com/users/jeromekelleher/gists{/gist_id}","starred_url":"https://api.github.com/users/jeromekelleher/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jeromekelleher/subscriptions","organizations_url":"https://api.github.com/users/jeromekelleher/orgs","repos_url":"https://api.github.com/users/jeromekelleher/repos","events_url":"https://api.github.com/users/jeromekelleher/events{/privacy}","received_events_url":"https://api.github.com/users/jeromekelleher/received_events","type":"User","site_admin":false},"created_at":"2015-09-10T11:27:23Z","updated_at":"2015-09-10T11:27:23Z","body":"Having gone through this in a bit of detail, I think it's a great starting point. There is a bit of work to be done I think in the overall layout, and particularly in how we run the queries and do the testing. However, I don't think these are major problems and we should be able to iterate towards something that could be merged quickly. Here are some high-level issues:\r\n\r\n**Data representation and input**\r\n\r\nWhere does the G2P `data set` live within the current data hierarchy? Is there an ``AssociationSet`` object (or whatever) which we can use to model the container for a given set of associations? Without this, it's difficult to fit the G2P model into the current outline, which expects a container. \r\n\r\nAssuming this is the case, and that an ``AssociationSet`` belongs to a ``Dataset``, we can then make a new directory that is akin to ``variants`` and ``reads`` in the data file system hierarchy. We then read files from this hierarchy, which defines the source for a particular association set. We should then be able to make a ``AbstractAssociationSet`` which extends ``DatamodelObject``, and fit in to the existing infrastructure much more neatly.\r\n\r\n** Testing **\r\n\r\nCurrently, test code is mixed in with production code, which we should avoid. The main reason for this I think is the lack of clarity on what constitutes a distinct source of data. Ideally, we would like\r\n\r\n1. Data driven tests, following the examples in ``tests/datadriven`` which take in specific data files stored in ``tests/data/`` and verify that the GA objects we produce from this intput are valid and correctly transformed into the GA4GH format.\r\n2. Random data tests. Randomly generated data is used extensively to test the API, and basically allows us to test everything that doesn't involve translating data (which is checked using the data driven tests outlined above). A method of generating random G2P data is therefore essential. The output doesn't have to be a realistic model in any way, it just has to represent a reasonable spread of possible output values and allow us to exercise the API without having huge data files lying around.\r\n\r\n** Client code **\r\n\r\nThere is no client API provided. This should be added and tested, so we can run queries from the command line without using curl or whatever.\r\n\r\nThese is my take in any case. I'm afraid I don't understand the G2P model very well, so I apologise for any stupid points I've made above. As I say, I think this is a great start and we should have a good implementation ready to merge soon."}}