-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TASK: Robust Metamist Integrations #784
base: main
Are you sure you want to change the base?
Conversation
Yes, but could we do it in a separate PR (merged before this one). If something goes wrong it's easier to track down the cause! |
…t API and implement timeout tests.
|
||
from cpg_utils import to_path | ||
from metamist.apis import AnalysisApi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Gross that we used to do this here.
from metamist.exceptions import ApiException | ||
from metamist.models import Analysis, AnalysisStatus | ||
|
||
from .metamist import AnalysisStatus, get_metamist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just need to keep an eye on this it case it creates any pickling issues. Let's try a test run later!
logging.error( | ||
f'Error: {e} Call {api_func} failed with payload:\n{str(kwargv)}', | ||
) | ||
# TODO: discuss should we catch all here as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
certainly a good idea.
return True | ||
|
||
|
||
def mock_aapi_update_analysis_timeout_fails(*args, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious what does aapi
mean ?
@retry( | ||
stop=stop_after_attempt(3), | ||
wait=wait_exponential(multiplier=3, min=8, max=30), | ||
retry=retry_if_exception_type(ServiceException), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way for us to be alerted if too many of these exceptions are raised?
This PR is implementing items for task Robust Metamist Integrations on the road map.
TODO
WIP, this PR latest coverage at 76%
Two functions in metamist are with comments 'not used anywhere' (process_existing_analysis, find_joint_calling_analysis), Should we remove? It would make test coverage at 91% after this PR.It seems we just need to differentiate between ServiceException and the rest of ApiException.
ServiceException can be retried or other we should stop.
Metamist timeouts killing pipelines #585
Error Details: It fails on
Exception:
production-pipelines/cpg_workflows/metamist.py
Line 318 in f4801c6
Solution: We are ging to keep the existing behaviour
production-pipelines/cpg_workflows/metamist.py
Line 635 in f4801c6