Add metrics module to DeepVariant Runner #19
Conversation
| import logging | ||
| import time | ||
| import uuid | ||
| import requests |
There was a problem hiding this comment.
Moving requests before time causes a lint error. I think because requests is considered a 3rd party module.
I added a new line before requests to make it clear it's in a new 'list'.
There was a problem hiding this comment.
Ah, I see! Yes, for 3rd party module we add them in a separate group. And a blank line between each group is the right way to go. Can you move from typing import Dict, Optional to the first group then?
There was a problem hiding this comment.
Moving that line to the first group caused a lint error, why do you think typing is not third party module?
There was a problem hiding this comment.
That's interesting. We don't have typing in the setup.py file (in Variant Transforms), so it should be python build in. I don't know whether it is different for Python 3. But if the lint complains, let's follow that.
There was a problem hiding this comment.
Thanks Allie for your comments and attention to details.
metrics.py
Outdated
| """Encapsulates information representing a Concord event.""" | ||
|
|
||
| def __init__(self, | ||
| event_name, |
There was a problem hiding this comment.
consider add type to increase readability.
| import logging | ||
| import time | ||
| import uuid | ||
| import requests |
There was a problem hiding this comment.
Moving requests before time causes a lint error. I think because requests is considered a 3rd party module.
I added a new line before requests to make it clear it's in a new 'list'.
metrics.py
Outdated
| """Encapsulates information representing a Concord event.""" | ||
|
|
||
| def __init__(self, | ||
| event_name, |
| import logging | ||
| import time | ||
| import uuid | ||
| import requests |
There was a problem hiding this comment.
Ah, I see! Yes, for 3rd party module we add them in a separate group. And a blank line between each group is the right way to go. Can you move from typing import Dict, Optional to the first group then?
metrics.py
Outdated
| _DEEP_VARIANT_RUN = 'DeepVariantRun' | ||
| _HTTP_REQUEST_TIMEOUT_SEC = 10 | ||
| _PYTHON = 'PYTHON' | ||
| _VIRTUAL_CHC_DEEPVARIANT = 'virtual.chc.deepvariant' |
There was a problem hiding this comment.
Is this name set in stone already? otherwise virtual.hcls.deepvariant would be preferable
There was a problem hiding this comment.
I am not really sure if changing this will affect anything in a bad way. I am running tests now and will update this comments as soon as I find out more.
| import requests | ||
| from typing import Dict, Optional, Text | ||
|
|
||
| _CLEARCUT_ENDPOINT = 'https://play.googleapis.com/log' |
There was a problem hiding this comment.
optional, but, in general, constants that are (1) only used once and (2) not expected to be tweaked could just be inlined.
So, for example, we are not likely to change anything in this list except maybe the request timeout
There was a problem hiding this comment.
I guess another benefit of having all these constant values here is the ease of finding them instead of scattered at different parts of code.
So if you don't mind I am going to keep them as is.
| import logging | ||
| import time | ||
| import uuid | ||
| import requests |
There was a problem hiding this comment.
That's interesting. We don't have typing in the setup.py file (in Variant Transforms), so it should be python build in. I don't know whether it is different for Python 3. But if the lint complains, let's follow that.
| import logging | ||
| import time | ||
| import uuid | ||
| import requests |
There was a problem hiding this comment.
Thanks Allie for your comments and attention to details.
| import requests | ||
| from typing import Dict, Optional, Text | ||
|
|
||
| _CLEARCUT_ENDPOINT = 'https://play.googleapis.com/log' |
There was a problem hiding this comment.
I guess another benefit of having all these constant values here is the ease of finding them instead of scattered at different parts of code.
So if you don't mind I am going to keep them as is.
metrics.py
Outdated
| _DEEP_VARIANT_RUN = 'DeepVariantRun' | ||
| _HTTP_REQUEST_TIMEOUT_SEC = 10 | ||
| _PYTHON = 'PYTHON' | ||
| _VIRTUAL_CHC_DEEPVARIANT = 'virtual.chc.deepvariant' |
There was a problem hiding this comment.
I am not really sure if changing this will affect anything in a bad way. I am running tests now and will update this comments as soon as I find out more.
samanvp
force-pushed
the
metrics
branch
2 times, most recently
from
239b0f8
to
91044eb
Compare
This PR contains the implementation of a decorator to collect metrics from DeepVariant Runner. A second PR will utilize the newly defined decorator to collect usage metrics.
This PR contains the implementation of a class to collect metrics from DeepVariant Runner.
A second PR will utilize the newly defined class in a decorator to collect usage metrics.