search

oncokb / oncokb-annotator

Annotates variants in MAF with OncoKB annotation.
GNU Affero General Public License v3.0
122 stars 61 forks source link

MafAnnotator (and the others too, probably) silently fails when token is expired #146

Closed fd-oncodna closed 2 years ago

fd-oncodna commented 2 years ago

Last month, our OncoKB token expired and we didn't notice. Worse, our pipelines seemingly kept working perfectly but the MafAnnotator was in fact failing silently.

My guess is that the condition (used at several places in the code) to throw the authorization exception is too narrow; an expired token probably results in receiving "'403" responses rather than "401" ones. 

if response.status_code == 401:
        raise Exception('unauthorized')

Maybe it would be a good idea to replace that with something like response.status_code > 400 or even use the builtin mechanism of the requests library response.raise_for_status()

zhx828 commented 2 years ago

Sounds good. I will look into. Thanks for the suggestion!

zhx828 commented 2 years ago

Looking back at the implementation, it seems to me that we only want to stop running the script when the API returns 401. For other errors, especially 500 code, we like to continue executing. This is especially helpful when the user is running a large dataset, one exception might cause the whole pipeline fails.

But in the meantime, we also do not provide any information when fail to annotated variant. This could create confusion and might lead to inaccurate data analysis. Here I propose the following, let me know what you think.

  1. introduce a new parameter '-e', dest='Skip API exception', default='false'. In this mode, all other exceptions will be skipped. Only the 401 will stop executing.
  2. We print the exception together with the variant in the log
  3. Add a column ANNOTATED before all other annotation columns. The value will be TRUE/FALSE. The API exception will be printed after FALSE in parenthesis. FALSE (exception info)
zhx828 commented 2 years ago

@jjgao what do you think above?

jjgao commented 2 years ago

@zhx828 wondering if we should have an endpoint to validate a token and always check that as the first step in our annotator?

zhx828 commented 2 years ago

After discussing with JJ, we decided to do the following. Feel free to comment if you have other suggestions.