[BEAM-2732][BEAM-4028] Logging relies on StateSampler for context #5356

pabloem · 2018-05-14T18:32:53Z

The Logging module will no longer implement its own context tracking for step and stage names.

Because it no longer plays part in high-performance operations, I'm removing it and its cython annotations.

Passing PostCommit: https://builds.apache.org/job/beam_PostCommit_Python_Verify/4985/

pabloem · 2018-05-14T18:36:41Z

Run Python Dataflow ValidatesRunner

pabloem · 2018-05-14T18:39:22Z

Run Python PostCommit

pabloem · 2018-05-15T21:48:18Z

Run Python PostCommit

pabloem · 2018-05-15T23:06:59Z

Run Python PostCommit

pabloem · 2018-05-17T00:12:45Z

Run Python PostCommit

pabloem · 2018-05-18T20:57:45Z

Run Python Dataflow ValidatesRunner

pabloem · 2018-06-04T17:21:43Z

This unifies context management in Python, which simplifies further feature work, and also expands the use of NameContext, which should improve the separation of runner and sdk harness.

pabloem · 2018-06-26T19:10:29Z

Run Python Dataflow ValidatesRunner

pabloem · 2018-06-26T22:06:45Z

Run Python PreCommit

pabloem · 2018-06-27T00:37:25Z

Run Python PreCommit

pabloem · 2018-06-27T01:12:33Z

Run Python PreCommit

pabloem · 2018-06-27T21:28:09Z

Run Python Dataflow ValidatesRunner

pabloem · 2018-06-28T00:25:16Z

Run Python Dataflow ValidatesRunner

pabloem · 2018-06-28T16:39:13Z

r: @charlesccychen
This unifies context management in Python, so that logging will use the state sampler to retrieve its current state. Also, NameContext is used more widely to improve step name management.

charlesccychen

Thanks Pablo! This is a great cleanup. Can you also run Robert's benchmarks here with Cython, with and without this change: #4741?

charlesccychen · 2018-06-29T19:08:39Z

sdks/python/apache_beam/runners/worker/operation_specs.py

    self.operations = operations
    self.stage_name = stage_name
-    # TODO(BEAM-4028): Remove arguments other than name_contexts.


Is this obsolete? The Jira is still open.

Oops. Not obsolete. Good catch!

charlesccychen · 2018-07-03T01:23:27Z

sdks/python/apache_beam/runners/common.py

@@ -539,19 +529,14 @@ def __init__(self,
      windowing: windowing properties of the output PCollection(s)
      tagged_receivers: a dict of tag name to Receiver objects
      step_name: the name of this step
-      logging_context: a LoggingContext object
+      logging_context: DEPRECATED


Can you add a JIRA to remove this?

Added BEAM-4728.

charlesccychen · 2018-07-03T01:25:28Z

sdks/python/apache_beam/runners/worker/bundle_processor.py

    self.windowed_coder = windowed_coder
    self.windowed_coder_impl = windowed_coder.get_impl()
-    self.step_name = step_name


Is this deletion intentional? If so, can you add a comment / JIRA reference to clean up step_name in the arguments?

This is part of my goal with BEAM-4028. The step name is meant to only be retrievable through the name context.

charlesccychen · 2018-07-03T01:26:39Z

sdks/python/apache_beam/runners/worker/logger.py

@@ -49,7 +48,7 @@ def get_data(self):
 per_thread_worker_data = _PerThreadWorkerData()


-class PerThreadLoggingContext(LoggingContext):
+class PerThreadLoggingContext(object):


Are we able to get rid of this class entirely? It looks like you removed the only usage in operations.py.

This class is used internally at google, so we need to remove it from there first.

charlesccychen · 2018-07-03T01:27:55Z

sdks/python/apache_beam/runners/worker/logger.py

@@ -34,7 +34,6 @@ class _PerThreadWorkerData(threading.local):

  def __init__(self):
    super(_PerThreadWorkerData, self).__init__()
-    # TODO(robertwb): Consider starting with an initial (ignored) ~20 elements


Accidental deletion?

The logging context will be removed once it's no longer useful (after it's removed from Google code) so optimizations should not be considered anymore. I'll remove it ASAP as part of BEAM-4728

charlesccychen · 2018-07-03T01:29:59Z

sdks/python/apache_beam/runners/worker/statesampler.py

@@ -62,12 +64,22 @@ def __init__(self, prefix, counter_factory,
               sampling_period_ms=DEFAULT_SAMPLING_PERIOD_MS):
    self.states_by_name = {}
    self._prefix = prefix
-    self._counter_factory = counter_factory
+    self._counter_factory = counter_factory or CounterFactory()


It looks like the second branch is only used by tests. Can we have the tests pass empty CounterFactory()s instead of adding this optional behavior in the actual code?

pabloem

Thanks Charles!

pabloem · 2018-07-03T21:42:43Z

sdks/python/apache_beam/runners/worker/logger.py

@@ -49,7 +48,7 @@ def get_data(self):
 per_thread_worker_data = _PerThreadWorkerData()


-class PerThreadLoggingContext(LoggingContext):
+class PerThreadLoggingContext(object):


This class is used internally at google, so we need to remove it from there first.

pabloem · 2018-07-03T22:07:44Z

sdks/python/apache_beam/runners/common.py

@@ -539,19 +529,14 @@ def __init__(self,
      windowing: windowing properties of the output PCollection(s)
      tagged_receivers: a dict of tag name to Receiver objects
      step_name: the name of this step
-      logging_context: a LoggingContext object
+      logging_context: DEPRECATED


Added BEAM-4728.

pabloem · 2018-07-03T22:16:33Z

sdks/python/apache_beam/runners/worker/bundle_processor.py

    self.windowed_coder = windowed_coder
    self.windowed_coder_impl = windowed_coder.get_impl()
-    self.step_name = step_name


This is part of my goal with BEAM-4028. The step name is meant to only be retrievable through the name context.

pabloem · 2018-07-03T22:17:56Z

sdks/python/apache_beam/runners/worker/logger.py

@@ -34,7 +34,6 @@ class _PerThreadWorkerData(threading.local):

  def __init__(self):
    super(_PerThreadWorkerData, self).__init__()
-    # TODO(robertwb): Consider starting with an initial (ignored) ~20 elements


The logging context will be removed once it's no longer useful (after it's removed from Google code) so optimizations should not be considered anymore. I'll remove it ASAP as part of BEAM-4728

pabloem · 2018-07-03T22:18:21Z

sdks/python/apache_beam/runners/worker/operation_specs.py

    self.operations = operations
    self.stage_name = stage_name
-    # TODO(BEAM-4028): Remove arguments other than name_contexts.


Oops. Not obsolete. Good catch!

pabloem · 2018-07-03T22:41:30Z

Also removed the extra path in statesampler for CounterFactory

pabloem · 2018-07-03T23:16:01Z

Run Python Dataflow ValidatesRunner

pabloem · 2018-07-03T23:29:33Z

Results of the map_fn_microbenchmark. For some (i'd think floating-point-related) reason, it gives per-element cost as zero, but if you observe row-per-row, the cost with changes is slightly lower than the cost on master.

On master:

     1 element  0.798268 sec
  1001 elements 1.01165 sec
  2001 elements 1.05419 sec
  3001 elements 1.1398 sec
  4001 elements 1.37623 sec
  5001 elements 1.47872 sec
  6001 elements 1.68769 sec
  7001 elements 1.68809 sec
  8001 elements 1.8503 sec
  9001 elements 2.0606 sec
Fixed cost   0.8104043092640963
Per-element  0.0
R^2          0.9845043457059202

With these changes:

     1 element  0.796835 sec
  1001 elements 0.952501 sec
  2001 elements 1.01314 sec
  3001 elements 1.17117 sec
  4001 elements 1.31416 sec
  5001 elements 1.3791 sec
  6001 elements 1.54986 sec
  7001 elements 1.68663 sec
  8001 elements 1.71841 sec
  9001 elements 1.93632 sec
Fixed cost   0.8011857992764676
Per-element  0.0
R^2          0.9922532858766798

charlesccychen · 2018-07-06T21:38:09Z

Thanks Pablo! This LGTM. Can you rebase? It looks like this is a great performance improvement too, cutting down processing overhead.

pabloem · 2018-07-09T15:53:31Z

Run Python Dataflow ValidatesRunner

pabloem · 2018-07-09T17:14:04Z

Tests passing. I'll squash and merge this after lunch.

pabloem · 2018-07-09T18:53:52Z

Squashed commits and resolved conflicts. Reruning tests.

pabloem · 2018-07-09T18:54:03Z

Run Python Dataflow ValidatesRunner

pabloem · 2018-07-09T19:47:00Z

Merged. Thanks @charlesccychen for reviewing the large-ish change : )

pabloem · 2018-08-15T21:24:34Z

This was meant to be removed, because optimizations are no longer needed. This list only ever contains one element per work item.

On Wed, Aug 15, 2018 at 2:23 PM Charles Chen ***@***.***> wrote: ***@***.**** commented on this pull request. In sdks/python/apache_beam/runners/worker/logger.py <#5356 (comment)>: > @@ -34,7 +34,6 @@ class _PerThreadWorkerData(threading.local): def __init__(self): super(_PerThreadWorkerData, self).__init__() - # TODO(robertwb): Consider starting with an initial (ignored) ~20 elements Part of this TODO was accidentally deleted. Please fix. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#5356 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABPc7Ot3bFoE0T5jPC3Z5l-RCPxci4NFks5uRJFZgaJpZM4T-S3o> .

-- Got feedback? go/pabloem-feedback

pabloem force-pushed the ss-logging branch from 8dd7017 to 108b77f Compare May 15, 2018 22:44

pabloem force-pushed the ss-logging branch from 439d60d to 0feccaf Compare May 17, 2018 00:12

pabloem force-pushed the ss-logging branch from 0feccaf to 1f2cfbe Compare May 18, 2018 20:57

pabloem changed the title ~~[DO NOT REVIEW] Logging relies on StateSampler for context~~ [BEAM-2732] Logging relies on StateSampler for context Jun 4, 2018

pabloem changed the title ~~[BEAM-2732] Logging relies on StateSampler for context~~ [BEAM-2732][BEAM-4028] Logging relies on StateSampler for context Jun 4, 2018

pabloem force-pushed the ss-logging branch 2 times, most recently from cead665 to 84f8fb6 Compare June 26, 2018 18:56

pabloem force-pushed the ss-logging branch from 84f8fb6 to a737d00 Compare June 27, 2018 00:38

pabloem force-pushed the ss-logging branch from a737d00 to 5b19da0 Compare June 27, 2018 18:44

charlesccychen reviewed Jul 3, 2018

View reviewed changes

pabloem commented Jul 3, 2018

View reviewed changes

pabloem force-pushed the ss-logging branch from 920b59b to 35b36a7 Compare July 9, 2018 15:36

Logging relies on StateSampler for context

9cf64f5

pabloem force-pushed the ss-logging branch from 35b36a7 to 9cf64f5 Compare July 9, 2018 18:53

pabloem merged commit f063b15 into apache:master Jul 9, 2018

pabloem deleted the ss-logging branch July 9, 2018 19:46

[BEAM-2732][BEAM-4028] Logging relies on StateSampler for context #5356

[BEAM-2732][BEAM-4028] Logging relies on StateSampler for context #5356

Conversation

pabloem commented May 14, 2018 • edited

pabloem commented May 14, 2018

pabloem commented May 14, 2018

pabloem commented May 15, 2018

pabloem commented May 15, 2018

pabloem commented May 17, 2018

pabloem commented May 18, 2018

pabloem commented Jun 4, 2018 • edited

pabloem commented Jun 26, 2018

pabloem commented Jun 26, 2018

pabloem commented Jun 27, 2018

pabloem commented Jun 27, 2018

pabloem commented Jun 27, 2018

pabloem commented Jun 28, 2018

pabloem commented Jun 28, 2018

charlesccychen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabloem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabloem commented Jul 3, 2018

pabloem commented Jul 3, 2018

pabloem commented Jul 3, 2018 • edited

charlesccychen commented Jul 6, 2018

pabloem commented Jul 9, 2018

pabloem commented Jul 9, 2018

pabloem commented Jul 9, 2018

pabloem commented Jul 9, 2018

pabloem commented Jul 9, 2018

pabloem commented Aug 15, 2018 via email

pabloem commented May 14, 2018 •

edited

pabloem commented Jun 4, 2018 •

edited

pabloem commented Jul 3, 2018 •

edited