[BEAM-2241] Renames some python classes and functions that were unnecessarily public #3036

chamikaramj · 2017-05-10T03:54:46Z

Adds a note to documentation of classes that are public but should be only used internally by the SDK (non-user facing classes).

Marks some of the modules as experimental.

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

Make sure the PR title is formatted like:
[BEAM-<Jira issue #>] Description of pull request
Make sure tests pass via mvn clean verify.
Replace <Jira issue #> in the title with the actual Jira issue
number, if there is one.
If this contribution is large, please file an Apache
Individual Contributor License Agreement.

chamikaramj · 2017-05-10T03:57:13Z

R: @aaltay

aaltay

Thank you @chamikaramj. This is great.

Given the size and pending release, adding another reviewer might be useful.

aaltay · 2017-05-10T04:03:22Z

sdks/python/apache_beam/coders/typecoders.py

@@ -72,7 +72,9 @@ def MakeXyzs(v):


 class CoderRegistry(object):
-  """A coder registry for typehint/coder associations."""
+  """For internal use only; no backwards-compatibility guarantees.


register_coder coder is public, this class is not internal.

Reverted this update.

aaltay · 2017-05-10T04:04:49Z

sdks/python/apache_beam/internal/gcp/auth.py

@@ -58,7 +60,9 @@ class AuthenticationException(retry.PermanentException):


 class GCEMetadataCredentials(OAuth2Credentials):


This could be just renamed to _ GCEMetadataCredentials all users of it should be in this file.

aaltay · 2017-05-10T04:08:16Z

sdks/python/apache_beam/metrics/cells.py

@@ -31,7 +31,9 @@


 class CellCommitState(object):


It does not look like there is much public interfaces in this file. Should we add __all__?

Added an empty all so that methods here will not be imported by an import *.

Seems like lint was crashing due to this change (empty all tag) for some reason. So had to revert this.

aaltay · 2017-05-10T04:10:59Z

sdks/python/apache_beam/metrics/execution.py

@@ -36,7 +36,9 @@


 class MetricKey(object):
-  """Key used to identify instance of metric cell.
+  """For internal use only; no backwards-compatibility guarantees.


Should we just update the comment at the top of the file that says Internal classes for Metrics API.

Top level comment should be enough. Updated.

aaltay · 2017-05-10T04:12:03Z

sdks/python/apache_beam/runners/common.py

@@ -72,7 +77,9 @@ def __init__(self, do_fn, method_name):


 class DoFnSignature(object):
-  """Represents the signature of a given ``DoFn`` object.
+  """For internal use only; no backwards-compatibility guarantees.


Would not these be useful for an implementer of a new runner?

Removed the comment from DoFnSignature, DoFnInvoker, and implementations of DoFnInvoker.

aaltay · 2017-05-10T04:13:24Z

sdks/python/apache_beam/runners/common.py

@@ -533,6 +550,7 @@ def counter_for(self, aggregator):

 # TODO(robertwb): Replace core.DoFnContext with this.


@sb2nov - Unrelated to PR. Is this another opportunity for removing backward compatibility code?

Thanks, I'll look into it.

aaltay · 2017-05-10T04:15:35Z

sdks/python/apache_beam/runners/dataflow/internal/apiclient.py

@@ -15,7 +15,9 @@
 # limitations under the License.
 #

-"""Dataflow client utility functions."""
+""" For internal use only. No backwards compatibility guarantees.


Is it enough to just add this comment here and not add the same comment to individual classes in this file?

For example iobase.py only comment is added to the top.

Removed from classes. Top level comment should be enough.

chamikaramj · 2017-05-10T04:33:09Z

R: @sb2nov

sb2nov · 2017-05-10T04:42:04Z

sdks/python/apache_beam/coders/coder_impl.py

@@ -97,7 +98,9 @@ def get_estimated_size_and_observables(self, value, nested=False):


 class SimpleCoderImpl(CoderImpl):
-  """Subclass of CoderImpl implementing stream methods using encode/decode."""
+  """For internal use only; no backwards-compatibility guarantees.


Just wanted to check if the DocString still renders correctly as I thought the first line acted as a summary.

So the thinking was that if the class should not be used by users, it might be good to have this as a summary. Also, I generated a doc using pydoc and this doesn't seem to be being rendered in a special way at least for that tool.

…lic. Adds a note to documentation of classes that are public but should be only used internally by the SDK (non-user facing classes). Marks some of the modules as experimental.

chamikaramj

Thanks. PTAL.

chamikaramj · 2017-05-10T04:36:39Z

sdks/python/apache_beam/coders/typecoders.py

@@ -72,7 +72,9 @@ def MakeXyzs(v):


 class CoderRegistry(object):
-  """A coder registry for typehint/coder associations."""
+  """For internal use only; no backwards-compatibility guarantees.


Reverted this update.

chamikaramj · 2017-05-10T04:37:36Z

sdks/python/apache_beam/internal/gcp/auth.py

@@ -58,7 +60,9 @@ class AuthenticationException(retry.PermanentException):


 class GCEMetadataCredentials(OAuth2Credentials):


chamikaramj · 2017-05-10T04:51:09Z

sdks/python/apache_beam/metrics/cells.py

@@ -31,7 +31,9 @@


 class CellCommitState(object):


Added an empty all so that methods here will not be imported by an import *.

chamikaramj · 2017-05-10T04:55:25Z

sdks/python/apache_beam/metrics/execution.py

@@ -36,7 +36,9 @@


 class MetricKey(object):
-  """Key used to identify instance of metric cell.
+  """For internal use only; no backwards-compatibility guarantees.


Top level comment should be enough. Updated.

chamikaramj · 2017-05-10T04:57:50Z

sdks/python/apache_beam/runners/common.py

@@ -72,7 +77,9 @@ def __init__(self, do_fn, method_name):


 class DoFnSignature(object):
-  """Represents the signature of a given ``DoFn`` object.
+  """For internal use only; no backwards-compatibility guarantees.


Removed the comment from DoFnSignature, DoFnInvoker, and implementations of DoFnInvoker.

chamikaramj · 2017-05-10T04:59:36Z

sdks/python/apache_beam/runners/dataflow/internal/apiclient.py

@@ -15,7 +15,9 @@
 # limitations under the License.
 #

-"""Dataflow client utility functions."""
+""" For internal use only. No backwards compatibility guarantees.


Removed from classes. Top level comment should be enough.

chamikaramj · 2017-05-10T05:02:44Z

sdks/python/apache_beam/coders/coder_impl.py

@@ -97,7 +98,9 @@ def get_estimated_size_and_observables(self, value, nested=False):


 class SimpleCoderImpl(CoderImpl):
-  """Subclass of CoderImpl implementing stream methods using encode/decode."""
+  """For internal use only; no backwards-compatibility guarantees.


So the thinking was that if the class should not be used by users, it might be good to have this as a summary. Also, I generated a doc using pydoc and this doesn't seem to be being rendered in a special way at least for that tool.

aaltay · 2017-05-10T05:46:48Z

LGTM (assuming the tests will pass).

sb2nov · 2017-05-10T06:06:21Z

LGTM

… unnecessarily public. Adds a note to documentation of classes that are public but should be only used internally by the SDK (non-user facing classes). Marks some of the modules as experimental.

aaltay reviewed May 10, 2017

View reviewed changes

sb2nov reviewed May 10, 2017

View reviewed changes

chamikaramj added 2 commits May 9, 2017 22:32

Renames some python classes and functions that were unnecessarily pub…

3c1136c

…lic. Adds a note to documentation of classes that are public but should be only used internally by the SDK (non-user facing classes). Marks some of the modules as experimental.

Addresses reviewer comments.

bba46a4

chamikaramj force-pushed the update_public_api branch from e6f90ec to bba46a4 Compare May 10, 2017 05:40

chamikaramj commented May 10, 2017

View reviewed changes

Fixes lint.

76f3f1a

asfgit closed this in 1fafaa8 May 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-2241] Renames some python classes and functions that were unnecessarily public #3036

[BEAM-2241] Renames some python classes and functions that were unnecessarily public #3036

chamikaramj commented May 10, 2017

chamikaramj commented May 10, 2017

aaltay left a comment

aaltay May 10, 2017

chamikaramj May 10, 2017

aaltay May 10, 2017

chamikaramj May 10, 2017

aaltay May 10, 2017

chamikaramj May 10, 2017

chamikaramj May 10, 2017

aaltay May 10, 2017

chamikaramj May 10, 2017

aaltay May 10, 2017

chamikaramj May 10, 2017

aaltay May 10, 2017

sb2nov May 10, 2017

aaltay May 10, 2017

chamikaramj May 10, 2017

chamikaramj commented May 10, 2017

sb2nov May 10, 2017

chamikaramj May 10, 2017

chamikaramj left a comment

chamikaramj May 10, 2017

chamikaramj May 10, 2017

chamikaramj May 10, 2017

chamikaramj May 10, 2017

chamikaramj May 10, 2017

chamikaramj May 10, 2017

chamikaramj May 10, 2017

aaltay commented May 10, 2017

sb2nov commented May 10, 2017

		@@ -58,7 +60,9 @@ class AuthenticationException(retry.PermanentException):


		class GCEMetadataCredentials(OAuth2Credentials):

		@@ -533,6 +550,7 @@ def counter_for(self, aggregator):

		# TODO(robertwb): Replace core.DoFnContext with this.

[BEAM-2241] Renames some python classes and functions that were unnecessarily public #3036

[BEAM-2241] Renames some python classes and functions that were unnecessarily public #3036

Conversation

chamikaramj commented May 10, 2017

chamikaramj commented May 10, 2017

aaltay 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

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

chamikaramj commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chamikaramj 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

aaltay commented May 10, 2017

sb2nov commented May 10, 2017