Add metrics module to DeepVariant Runner #19

samanvp · 2019-06-28T16:44:15Z

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.

allieychen

Thanks Saman! It looks great!

metrics.py

allieychen · 2019-06-28T16:56:14Z

metrics.py

+import logging
+import time
+import uuid
+import requests


nit: please keep it sorted.

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'.

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?

Moving that line to the first group caused a lint error, why do you think typing is not third party module?

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.

Thanks Allie for your comments and attention to details.

allieychen · 2019-06-28T16:58:03Z

metrics.py

+  """Encapsulates information representing a Concord event."""
+
+  def __init__(self,
+               event_name,


consider add type to increase readability.

metrics.py

samanvp

Thanks Allie for your comments.

metrics.py

samanvp · 2019-07-02T14:52:04Z

metrics.py

+import logging
+import time
+import uuid
+import requests


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

samanvp · 2019-07-03T13:41:11Z

metrics.py

+  """Encapsulates information representing a Concord event."""
+
+  def __init__(self,
+               event_name,


metrics.py

allieychen

Thanks Saman! I have added some comments for the test.

allieychen · 2019-07-03T19:27:43Z

metrics.py

+import logging
+import time
+import uuid
+import requests


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

metrics_test.py

kemp-google

Just nits

kemp-google · 2019-07-09T15:05:10Z

metrics.py

+_DEEP_VARIANT_RUN = 'DeepVariantRun'
+_HTTP_REQUEST_TIMEOUT_SEC = 10
+_PYTHON = 'PYTHON'
+_VIRTUAL_CHC_DEEPVARIANT = 'virtual.chc.deepvariant'


Is this name set in stone already? otherwise virtual.hcls.deepvariant would be preferable

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.

kemp-google · 2019-07-09T15:07:38Z

metrics.py

+import requests
+from typing import Dict, Optional, Text
+
+_CLEARCUT_ENDPOINT = 'https://play.googleapis.com/log'


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

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.

allieychen

Thanks Saman!

allieychen · 2019-07-09T15:07:41Z

metrics.py

+import logging
+import time
+import uuid
+import requests


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.

samanvp

Thank you both.
As soon as I get any info from our metrics I will update here.

samanvp · 2019-07-09T19:14:24Z

metrics.py

+import logging
+import time
+import uuid
+import requests


Thanks Allie for your comments and attention to details.

samanvp · 2019-07-09T19:20:50Z

metrics.py

+import requests
+from typing import Dict, Optional, Text
+
+_CLEARCUT_ENDPOINT = 'https://play.googleapis.com/log'


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.

samanvp · 2019-07-09T19:32:51Z

metrics.py

+_DEEP_VARIANT_RUN = 'DeepVariantRun'
+_HTTP_REQUEST_TIMEOUT_SEC = 10
+_PYTHON = 'PYTHON'
+_VIRTUAL_CHC_DEEPVARIANT = 'virtual.chc.deepvariant'


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.

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.

samanvp requested a review from allieychen June 28, 2019 16:44

allieychen reviewed Jun 28, 2019

View reviewed changes

samanvp force-pushed the metrics branch from b2b015e to 9243b15 Compare July 3, 2019 14:18

samanvp commented Jul 3, 2019

View reviewed changes

samanvp force-pushed the metrics branch from 9243b15 to fed80f0 Compare July 3, 2019 15:12

allieychen reviewed Jul 3, 2019

View reviewed changes

samanvp force-pushed the metrics branch from 32017dd to 6de9027 Compare July 8, 2019 15:52

kemp-google reviewed Jul 9, 2019

View reviewed changes

allieychen approved these changes Jul 9, 2019

View reviewed changes

samanvp commented Jul 9, 2019

View reviewed changes

samanvp force-pushed the metrics branch 2 times, most recently from 239b0f8 to 91044eb Compare July 11, 2019 22:30

samanvp added 5 commits July 18, 2019 14:23

Add metrics module to DeepVariant Runner

ec4c557

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.

First round of comments (Metrics)

ae775d0

Second round of comments (Metrics)

2aaddc9

Third round of comments (Metrics)

fcc3426

modify console_type to CLOUD_HCLS_OSS

9c6c110

samanvp force-pushed the metrics branch from 91044eb to 9c6c110 Compare July 18, 2019 19:25

samanvp mentioned this pull request Jul 18, 2019

Metrics utilization #25

Open

Add metrics module to DeepVariant Runner #19

Are you sure you want to change the base?

Add metrics module to DeepVariant Runner #19

Uh oh!

Conversation

samanvp commented Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allieychen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samanvp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

allieychen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kemp-google left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allieychen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samanvp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

samanvp commented Jun 28, 2019 •

edited

Loading