[BEAM-2724] Preparing statesampler to work with structured names#3786

Closed

pabloem wants to merge 7 commits intoapache:masterfrom

pabloem:ssampler-structured

Member

pabloem commented Aug 29, 2017

No description provided.

coveralls commented Aug 30, 2017

Coverage remained the same at 69.968% when pulling 9375dd6 on pabloem:ssampler-structured into 6280d49 on apache:master.

Member Author

pabloem commented Aug 30, 2017

Note: This PR is not yet ready for review and merge.

pabloem force-pushed the ssampler-structured branch 2 times, most recently from fc6a119 to 9e6f3c0 Compare

September 21, 2017 16:15

pabloem changed the title ~~Preparing statesampler to work with structured names~~ [BEAM-2724] Preparing statesampler to work with structured names

coveralls commented Sep 21, 2017

Coverage decreased (-0.01%) to 69.546% when pulling 9e6f3c0 on pabloem:ssampler-structured into a92c45f on apache:master.

Member Author

pabloem commented Sep 21, 2017

r: @charlesccychen
cc: @bjchambers
This changes the state sampler to rely on structured names for MSEC counters.

The new io_target argument allows states to track time spent in IO such as side inputs, shuffle and state. Tests have passed , and the latest commit only updates documentation.

coveralls commented Sep 21, 2017

Coverage decreased (-0.02%) to 69.54% when pulling b3d629f on pabloem:ssampler-structured into a92c45f on apache:master.

coveralls commented Sep 25, 2017

Coverage decreased (-0.03%) to 69.525% when pulling b6e7964 on pabloem:ssampler-structured into a92c45f on apache:master.

charlesccychen reviewed

View reviewed changes

Contributor

charlesccychen left a comment

Thanks!

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

+                                               stage_name=self.prefix,
+                                               step_name=step_name,
+                                               io_target=io_target)
+                    scoped_state = self.scoped_states_by_name.get(counter_name, None)

Contributor

charlesccychen Sep 25, 2017

We are conflating the type of what can be used as keys in self.scoped_states_by_name. Previously, this was just strings, but now, you are using this CounterName object, which doesn't seem correct. The CounterName class doesn't define __hash__() or even __cmp__() / __eq__(), so it is possible for you to create two semantically identical CounterName objects which do not compare as equal, and are not considered the same key by the self.scoped_states_by_name dict object.

This is related to the comment above noting that sometimes counter_name in this code is a CounterName, and at other times, it is a string. We should make this code unambiguous in this respect.

Member Author

pabloem Sep 25, 2017

The CounterName class is a subclass of a namedtuple (which, itself contains only strings or other namedtuples) - therefore it does implement the __hash__, __eq__, and __cmp__. So two objects that are semantically equal will hash to the same value. So this should be safe.

Does that make sense? What do you think?

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

+                  if state_name is None:
+                    # If state_name is None, the worker is still using old style
+                    # msec counters.
+                    counter_name = '%s-%s-msecs' % (self.prefix, step_name)

Contributor

charlesccychen Sep 25, 2017

Shouldn't this be '%s%s-msecs' to preserve the old behavior?

Also, there is some confusion in this code, in this legacy case where only step_name is provided, in what is used to index into self.scoped_states_by_name (i.e., is it step_name as you do on line 203, or is it counter_name, as done on line 223?).

It is also a little messy that counter_name can be either a string (this branch) or a CounterName object (the else branch)...

Member Author

pabloem Sep 25, 2017 •

edited

Loading

You can see on line 117 that we're chopping off the dash from prefixes, so now we add the dash here. I'm doing this to add the dash-free prefix to the CounterStructuredName that's reported to the service. This is only necessary while the clients stop using the legacy path.
You are completely right about the confusion. Good catch. I've updated the code to reflect this.
I understand. This is temporary while all the clients start providing state_name. This should take less than a couple weeks once this change is pulled in - and then the legacy path can be removed fully.

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

-                def scoped_state(self, name):
-                  """Returns a context manager managing transitions for a given state."""
-                  cdef ScopedState scoped_state = self.scoped_states_by_name.get(name, None)
+                def scoped_state(self, step_name, state_name=None, io_target=None):

Contributor

charlesccychen Sep 25, 2017

Please add TODO to make state_name a required parameters after all callers have migrated.

Member Author

pabloem Sep 25, 2017

Done.

sdks/python/apache_beam/runners/worker/statesampler.pyx Outdated

+                    counter_name = '%s-%s-msecs' % (self.prefix, step_name)
+                    scoped_state = self.scoped_states_by_name.get(step_name, None)
+                  else:
+                    counter_name = CounterName(state_name+'-msecs',

Contributor

charlesccychen Sep 25, 2017

nit: space before and after +.

Member Author

pabloem Sep 25, 2017

Done.

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

                     scoped_state.nsecs = 0
                     pythread.PyThread_release_lock(self.lock)
-                    self.scoped_states_by_name[name] = scoped_state
+                    self.scoped_states_by_name[counter_name] = scoped_state

Contributor

charlesccychen Sep 25, 2017

Please see above comments regarding the actual type of counter_name.

Member Author

pabloem Sep 25, 2017

Addressed.

pabloem commented

View reviewed changes

Member Author

pabloem left a comment

Comments addressed. Thanks for your time. Let me know what you think.

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

-                def scoped_state(self, name):
-                  """Returns a context manager managing transitions for a given state."""
-                  cdef ScopedState scoped_state = self.scoped_states_by_name.get(name, None)
+                def scoped_state(self, step_name, state_name=None, io_target=None):

Member Author

pabloem Sep 25, 2017

Done.

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

+                  if state_name is None:
+                    # If state_name is None, the worker is still using old style
+                    # msec counters.
+                    counter_name = '%s-%s-msecs' % (self.prefix, step_name)

Member Author

pabloem Sep 25, 2017 •

edited

Loading

You can see on line 117 that we're chopping off the dash from prefixes, so now we add the dash here. I'm doing this to add the dash-free prefix to the CounterStructuredName that's reported to the service. This is only necessary while the clients stop using the legacy path.
You are completely right about the confusion. Good catch. I've updated the code to reflect this.
I understand. This is temporary while all the clients start providing state_name. This should take less than a couple weeks once this change is pulled in - and then the legacy path can be removed fully.

sdks/python/apache_beam/runners/worker/statesampler.pyx Outdated

+                    counter_name = '%s-%s-msecs' % (self.prefix, step_name)
+                    scoped_state = self.scoped_states_by_name.get(step_name, None)
+                  else:
+                    counter_name = CounterName(state_name+'-msecs',

Member Author

pabloem Sep 25, 2017

Done.

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

+                                               stage_name=self.prefix,
+                                               step_name=step_name,
+                                               io_target=io_target)
+                    scoped_state = self.scoped_states_by_name.get(counter_name, None)

Member Author

pabloem Sep 25, 2017

The CounterName class is a subclass of a namedtuple (which, itself contains only strings or other namedtuples) - therefore it does implement the __hash__, __eq__, and __cmp__. So two objects that are semantically equal will hash to the same value. So this should be safe.

Does that make sense? What do you think?

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

                     scoped_state.nsecs = 0
                     pythread.PyThread_release_lock(self.lock)
-                    self.scoped_states_by_name[name] = scoped_state
+                    self.scoped_states_by_name[counter_name] = scoped_state

Member Author

pabloem Sep 25, 2017

Addressed.

charlesccychen approved these changes

View reviewed changes

Contributor

charlesccychen left a comment

Thanks, this LGTM after nit.

sdks/python/apache_beam/runners/worker/statesampler.pyx Outdated

-                def scoped_state(self, name):
-                  """Returns a context manager managing transitions for a given state."""
-                  cdef ScopedState scoped_state = self.scoped_states_by_name.get(name, None)
+                # TODO(pabloem) - Make state_name required once all callers migrate,

Contributor

charlesccychen Sep 25, 2017

nit: Can you change this to follow the project style of # TODO(XXX): YYY?

charlesccychen reviewed

View reviewed changes

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

+                                               stage_name=self.prefix,
+                                               step_name=step_name,
+                                               io_target=io_target)
+                    scoped_state = self.scoped_states_by_name.get(counter_name, None)

Contributor

charlesccychen Sep 25, 2017

pabloem wrote:
The CounterName class is a subclass of a namedtuple (which, itself contains only strings or other namedtuples) - therefore it does implement the __hash__, __eq__, and __cmp__. So two objects that are semantically equal will hash to the same value. So this should be safe.

Does that make sense? What do you think?

Thanks! I missed the inheritance--this makes sense. It will be much less confusing once we remove the first branch so that we can say that self.scoped_states_by_name is always keyed by CounterName.

Contributor

charlesccychen commented Sep 25, 2017

R: @bjchambers for merge

pabloem closed this

pabloem reopened this

pabloem closed this

pabloem reopened this

pabloem closed this

pabloem reopened this

coveralls commented Sep 27, 2017

Coverage decreased (-0.02%) to 69.537% when pulling 8cf66aa on pabloem:ssampler-structured into a92c45f on apache:master.

pabloem closed this

pabloem reopened this

pabloem closed this

pabloem reopened this

pabloem force-pushed the ssampler-structured branch from 8cf66aa to 1454095 Compare

September 27, 2017 17:34

coveralls commented Sep 28, 2017

Coverage increased (+0.03%) to 69.572% when pulling 1454095 on pabloem:ssampler-structured into 41239d8 on apache:master.

pab-goog added 3 commits

October 2, 2017 13:25


          Preparing statesampler to work with structured names

efa2b71


          Support iotarget for ssampler

1058fcd


          Improving documentation

91e7c93

pab-goog added 4 commits

October 2, 2017 13:25


          Fix missing arg

202202e


          Addressing comments

9cf96ab


          Fix nit

8bdb6c6


          Fix typo

29829fe

pabloem force-pushed the ssampler-structured branch from 1454095 to 29829fe Compare

October 2, 2017 20:26

Member Author

pabloem commented Oct 2, 2017

Run Python PostCommit

Member Author

pabloem commented Oct 2, 2017

jenkins: retest this please

coveralls commented Oct 3, 2017

Coverage remained the same at 69.568% when pulling 29829fe on pabloem:ssampler-structured into 4a5b3c0 on apache:master.

asfgit closed this in

f9bc763

pabloem deleted the ssampler-structured branch

October 3, 2017 18:08

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet