V0.4 #92

sdelatorrep · 2017-05-02T12:31:47Z

v0.4 proposal

mbaudis · 2017-05-02T12:37:12Z

src/main/proto/ga4gh/beacon.proto

@@ -226,9 +255,12 @@ message BeaconAlleleResponse {
  // Allele request as interpreted by the beacon. 
  BeaconAlleleRequest allele_request = 4;

+  // Version of the API provided by the beacon. 
+  string api_version = 5;


I think the convention is not to change attribute <-> index assignments, if the number has been used. The order in the spec doesn't matter. So it should be string api_version = 6.

Very good point. I didn't notice that, my mistake. Fixing it.

mbaudis · 2017-05-02T12:37:55Z

+1, after considering my comment on the numbering in .proto

antbro · 2017-05-02T12:47:36Z

Thanks It seems the data use conditions part has reverted back to just Consent Codes, whereas this was suppose to be flexible to allow for Consent Codes or ADA-M profiles (or anything else someone might want to state) or all the above. We also never decided whether this should be one design that could accommodate all these models, or different designs that could slot into one general or multiple specific placeholders So I think this needs a lot more thinking about I cc Francis who offered to lead on this aspect Cheers Tony sdelatorrep wrote:

…

v0.4 proposal ------------------------------------------------------------------------ You can view, comment on, or merge this pull request online at: #92 Commit Summary * Issue #55 * Issue #22 (add data use condition support) * Issue #59 (make reference_bases optional) * Issue #56 (filter dataset response) * Issue #12 (add apiVersion) * Issue #54 (error codes & messages) File Changes * *M* src/main/proto/ga4gh/beacon.proto <https://github.com/ga4gh/beacon-team/pull/92/files#diff-0> (48) Patch Links: * https://github.com/ga4gh/beacon-team/pull/92.patch * https://github.com/ga4gh/beacon-team/pull/92.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#92>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI_EVOM0mDmVYOU0c2mWyxzgr2mzAt69ks5r1yI0gaJpZM4NOFFJ>.

sdelatorrep · 2017-05-02T12:59:55Z

Hi @antbro . You're absolutely right but Consent Codes is the only one implemented as far as I know, so we thought it was better to include it in v0.4 and add ADA-M later when it's done. But the most important point of this PR is to resume some discussions that have been forgotten for a while (like this one you just raised).

antbro · 2017-05-02T13:21:18Z

OK. It seems that "implemented <https://github.com/ga4gh/ga4gh-consent-policy>" just means listing the codes on Avro. Do you know who actually made that github entry? Since ADA-M has formal serialisations in JSON and key:Value text, then perhaps Francis could 'implement' this too rather quickly. I believe the hold up on that has been getting a place in guithub to do it (we've made many requests and seemingly been ignored) Beyond that, I think we need to discuss these data use condition APIs more deeply, as just having a list of codes leaves a lot of important aspects unstated. Cheers Tony sdelatorrep wrote:

…

Hi @antbro <https://github.com/antbro> . You're absolutely right but Consent Codes is the only one implemented <https://github.com/ga4gh/ga4gh-consent-policy> as far as I know, so we thought it was better to include it in v0.4 and add ADA-M later when it's done. But the most important point of this PR is to resume some discussions that have been forgotten for a while (like this one you just raised). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#92 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI_EVGtuNC_Okmx-m8m6pVZibSO1zd01ks5r1yjMgaJpZM4NOFFJ>.

jrambla · 2017-05-03T16:22:47Z

+1 to its current status.

mbaudis · 2017-05-11T18:28:07Z

Merging this? @mcupak?

mcupak · 2017-05-11T18:30:41Z

src/main/proto/ga4gh/beacon.proto

@@ -16,8 +16,8 @@ message BeaconAlleleRequest {
  // Accepted values: non-negative integers smaller than reference length.
  int64 start = 2;

-  // Reference bases for this variant (starting from `start`).
-  //
+  // Reference bases for this variant (starting from `start`). OPTIONAL.


I wonder if this needs clarification. It's not really optional from the querier (request) perspective, it's from the beacon perspective. If a beacon doesn't store reference bases, it cannot reply to a question with reference bases, right? And if it does store reference bases, a query without them might be ambiguous. I think we should either describe how a beacon should behave in these situations, or have a field under the info endpoint disclosing what the beacon supports.

The most stringent way would be to have Beacons returning a verbose error if they do not support reference bases.

// If a value is provided here, a Beacon should return an error message if it does not // support reference bases for variant annotations.

Interestingly, ga4gh-schemas are not explicit about this.

Hi Miro,
The issue #59 has plenty of +1's and yourself seem to suggest making it optional...
Are you just asking for a better clarification of the schema comments?

UPDATE: #59 (comment)

mcupak · 2017-05-11T18:30:48Z

src/main/proto/ga4gh/beacon.proto

+  }
+
+  // If include_dataset_responses is true, use this field to filter the response.
+  FilterDatasetResponse filter_response = 8;


I don't know if I like this approach. If there are only 2 options, it should be a boolean instead of an enum. We also already have a field include_dataset_responses, which is related, so maybe the best thing is to have one filter field (e.g. include_dataset_responses=all|true|false|none). Ideally the name of the field would also explain we're filtering on exists. Thoughts?

I prefer explicit names than true/false values when the meaning is not obvious, as in this case. But I agree that we can reuse include_dataset_responses and convert it to a string which accepts the values: all|hit|miss|none (the latter being the default value if the field is omitted). We can update #56 with the final decision.

+1 to all|hit|miss|none in include_dataset_responses

Comments from the workshop yesterday:

include null in the filter

come up with a nice way to filter and name the parameter

mcupak · 2017-05-11T18:30:59Z

src/main/proto/ga4gh/beacon.proto

@@ -85,7 +93,7 @@ message BeaconDataset {

 // Organization owning a beacon. 
 message BeaconOrganization {
-  // Unique identifier of the organization. 
+  // Unique identifier of the organization. Use reversed namespace convention (e.g. org.ga4gh)


I'd replace "reversed namespace convention" with "reverse domain name notation", as that's the common term for this. Also missing punctuation and should hard-wrap to 80 characters (https://github.com/ga4gh/beacon-team/blob/develop/CONTRIBUTING.md#syntax-style-and-conventions).

+1 on @mcupak comment.

mcupak · 2017-05-11T18:31:03Z

src/main/proto/ga4gh/beacon.proto

@@ -113,10 +121,10 @@ message BeaconOrganization {

 // Beacon. 
 message Beacon {
-  // Unique identifier of the beacon. 
+  // Unique identifier of the beacon. Use reversed namespace convention (e.g. org.ga4gh.beacon)


Same as the corresponding field under BeaconOrganization.

mcupak · 2017-05-11T18:31:06Z

src/main/proto/ga4gh/beacon.proto

  string id = 1;

-  // Name of the beacon. 
+  // Name of the beacon. )


Typo (closing bracket with no opening).

mcupak · 2017-05-11T18:31:16Z

I like it overall, but I've noted a few suggestions under the corresponding lines. It would be much better to have separate PRs for the individual issues for easier discussion.

mcupak · 2017-05-11T18:32:28Z

src/main/proto/ga4gh/beacon.proto

+  // - HTTP error code 412: Missing mandatory parameters: referenceName, position/start and/or assemblyId
+  // - HTTP error code 412: The reference genome of this dataset X does not match the provided value
+  // - HTTP error code 412: Invalid alternateBases parameter, it can only be [ACTG]+
+  // - HTTP error code 412: Invalid referenceBases parameter, it can only be [ACTG]+


Lines too long.

mcupak · 2017-05-11T18:38:08Z

src/main/proto/ga4gh/beacon.proto

+    NOT_FOUND = 2; // HTTP error code 404
+    PRECONDITION_FAILED = 3; // HTTP error code 412
+  }
+
  // Numeric error code. 
  int32 error_code = 1;


If this is int, then the enum above is not used and should be removed, right? I think it's fine without the enum and just recommend values in the comment, like below.

This is a mistake. It should be: ErrorCode error_code = 1;
The idea is to have a common list of errors. If a new error code is required, initially GENERIC_ERROR will be used by the programmer and, later, this new error will be included in the specification.

mcupak · 2017-05-11T18:39:52Z

src/main/proto/ga4gh/beacon.proto

-  // Error message. 
+  // Error message.
+  // Accepted values:
+  // - HTTP error code 400: generic error. If not 


Unfinished sentence?

Yes... I'll fix it.

mcupak · 2017-05-11T18:42:07Z

src/main/proto/ga4gh/beacon.proto

+  // Error message.
+  // Accepted values:
+  // - HTTP error code 400: generic error. If not 
+  // - HTTP error code 401: Unauthenticated users cannot access this dataset


403 is more common. It looks like 401 is for something very specific in HTTP.

Hi Miro,

According to this page (one between many) https://en.wikipedia.org/wiki/HTTP_403...
Status codes 401 (Unauthorized) and 403 (Forbidden) have distinct meanings.
A 401 response indicates that access to the resource is restricted, and the request did not provide any HTTP authentication. It is possible that a new request for the same resource will succeed if authentication is provided. The response must include an HTTP WWW-Authenticate header to prompt the user-agent to provide credentials.
A 403 response generally indicates one of two conditions:
Authentication was provided, but the authenticated user is not permitted to perform the requested operation.
The operation is forbidden to all users. For example, requests for a directory listing return code 403 when directory listing has been disabled.

mcupak · 2017-05-11T18:43:30Z

src/main/proto/ga4gh/beacon.proto

+  // - HTTP error code 412: Missing mandatory parameters: referenceName, position/start and/or assemblyId
+  // - HTTP error code 412: The reference genome of this dataset X does not match the provided value
+  // - HTTP error code 412: Invalid alternateBases parameter, it can only be [ACTG]+
+  // - HTTP error code 412: Invalid referenceBases parameter, it can only be [ACTG]+


Maybe 400 Bad Request would be better for all of the 412, if we're mapping to HTTP?

mcupak · 2017-05-11T18:45:36Z

src/main/proto/ga4gh/beacon.proto

  // Numeric error code. 
  int32 error_code = 1;

-  // Error message. 
+  // Error message.
+  // Accepted values:


I'd probably just refer to these as standard values but allow for custom messages and codes added by beacons as they see fit. WDYT?

The idea is to standardise the error codes and messages (#54). If a programmer requires a new message, then s/he should use error_code=GENERIC_ERROR and set their custom message in the message field (maybe this should be clarified in the comments). Next releases of the API specification might include these new error messages.

I rather prefer to have fixed messages to avoid custom messages to re-interpret or add confusion. If the "official" message is not clear enough, my guess is that we should improve it.

mcupak · 2017-06-20T11:42:06Z

Merging the PR as this is a big step in the right direction. Let's clean up the spec in separate PRs.

sdelatorrep added 6 commits April 24, 2017 14:10

Issue ga4gh-beacon#55

c370e3f

Issue ga4gh-beacon#22 (add data use condition support)

57ff2a0

Issue ga4gh-beacon#59 (make reference_bases optional)

f17329c

Issue ga4gh-beacon#56 (filter dataset response)

7df78ef

Issue ga4gh-beacon#12 (add apiVersion)

ce03863

Issue ga4gh-beacon#54 (error codes & messages)

8c490b7

mbaudis reviewed May 2, 2017

View reviewed changes

Do not change attribute-index assignments.

313f15e

sdelatorrep added 2 commits May 2, 2017 17:04

Remove CC until further discussion on integrating both CC and ADA-M.

91a115b

Remove import.

389e05c

mcupak reviewed May 11, 2017

View reviewed changes

Fix typos and apply suggestions from comments.

dd35ebc

mcupak merged commit f17d295 into ga4gh-beacon:develop-proto Jun 20, 2017

This was referenced Jul 4, 2017

Add an “apiVersion” field to BeaconAlleleResponse #12

Closed

Add data use condition support #22

Closed

V0.4 #92

V0.4 #92

Conversation

sdelatorrep commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbaudis commented May 2, 2017 • edited Loading

antbro commented May 2, 2017 via email

sdelatorrep commented May 2, 2017

antbro commented May 2, 2017 via email

jrambla commented May 3, 2017

mbaudis commented May 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdelatorrep May 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcupak commented May 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcupak commented Jun 20, 2017

mbaudis commented May 2, 2017 •

edited

Loading

sdelatorrep May 12, 2017 •

edited

Loading