search

KarchinLab / open-cravat

A modular annotation tool for genomic variants
MIT License
113 stars 27 forks source link

ModuleInfoCache.update_remote() should be elevate the contents of the HttpError rather than throwing them away #121

Closed tenzinhl closed 1 year ago

tenzinhl commented 2 years ago

Currently store_utils.get_file_to_string() has a catch all except: clause that just returns an empty string when something went wrong with the request. This hid the true nature of the issue when I was running into issues trying to run oc module install-base, which was a simple SSL verification failure due to not having registered the proper certs with the requests library in the docker container.

The interaction that kind of misled me was in admin_util.ModuleInfoCache.update_remote() where all the message says is "please check internet connection", which was not my issue (and for many possible exceptions raised by requests not the issue either).

I would suggest removing the try-catch and just letting any raised exceptions be handled by the calling code. At the very least when it goes unhandled the exception (at least in my case) would have more useful information for debugging the issue.

Understandably this method is called in more than just one spot though, so curious what others' thoughts are on this. I quickly looked for where this function is called and there aren't too many sites.

tenzinhl commented 2 years ago

I was originally going to write a PR to make this suggested change, but since I'm a new user to OpenCRAVAT (and thus even greener of a dev) wasn't sure what other side effects it'd have.

kmoad commented 2 years ago

Hi tenzinhl. Thanks for the detailed digging here. I agree with letting exceptions bubble up from get_file_to_string, and handling them at a higher level. Catching everything into a return "" is icky. I have to read through the code a bit to see what the consequences are though.

If you'd like, we could split this up where you send in a PR that changes get_file_to_string to properly elevate exceptions, and I can fix the calling code.

Open to any suggestions you have.

tenzinhl commented 2 years ago

Hey! Sorry for the late update. PR is open now: https://github.com/KarchinLab/open-cravat/pull/123

Not many changes there, just has the exception bubble up. Places where the method is called will probably need to be modified for the new contract.

kmoad commented 2 years ago

Added a couple comments to the pr

kmoad commented 1 year ago

The PR. Along with commits 36e514b6a48ca080e28ebb7e7df6febbb2523378 and d711d154bb0f7632399c3648ad627067b7786ffd should fix this issue. They also improve oc's handling for fatal exceptions.