[BEAM-3761]Fix Python 3 cmp usage #4774

holdenk · 2018-02-28T22:01:24Z

Fixes our usage of cmp as part of Python 3 port. In some places where cmp was not being used remove the function entirely, in others rewrite to use total_ordering annotation.

Follow this checklist to help us incorporate your contribution quickly and easily:

Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue.
[X ] Write a pull request description that is detailed enough to understand:
- [ X] What the pull request does
- [ X ] Why it does it
- [ X ] How it does it
- [ X ] Why this approach
[ X ] Each commit in the pull request should have a meaningful subject line and body.
Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically. (ran python tests instead)
If this contribution is large, please file an Apache Individual Contributor License Agreement.

holdenk · 2018-02-28T22:59:06Z

cc @aaltay

holdenk · 2018-03-01T01:47:59Z

cc @robertwb

aaltay · 2018-03-02T01:34:02Z

sdks/python/apache_beam/io/gcp/datastore/v1/helper.py

 # pylint: enable=ungrouped-imports


+@deprecated(since="v2.4")


Why are we deprecating these functions?

So they encourage cmp like behaviour and I didn't see them used internally. This is as a separate commit, I can keep them non-deprecated and just use the rewrite for the internal cmp usage.

They are in the public API surface, it may have users. Let's not deprecate.

For the counterargument see What's new in Python 3: The cmp() function should be treated as gone, and the __cmp__() special method is no longer supported.

I do not follow the counterargument. These two are static functions for comparing datastore keys and paths. Providing these functions should be orthogonal to having cmp functions in the language or not. We can deprecate this if we think that they do not provide value to users, but the decision should not depend on the availability of cmp.

We can avoid deprecating them for now and revisit it later.

aaltay · 2018-03-02T01:42:25Z

sdks/python/apache_beam/testing/test_stream.py

+    return (type(self) is type(other) and
+            self.timestamped_values == other.timestamped_values)
+
+  def __ne__(self, other):


Do you need to implement ne, is not this the default implementation?

@holdenk what do you think?

In Python3 we are fine but in Python 2.7, ne isn't inherited from object. If you see the docs for total_ordering. See the notes in https://bugs.python.org/issue25732

aaltay · 2018-03-02T01:45:18Z

sdks/python/apache_beam/testing/test_stream.py


 from abc import ABCMeta
-from abc import abstractmethod
+from functools import total_ordering


@charlesccychen

Where do we compare these events?

aaltay · 2018-03-02T01:48:38Z

sdks/python/apache_beam/transforms/window.py

+  def __ne__(self, other):
+    return not self.__eq__(other)
+
+  def __lt__(self, other):


I think a type mismatch should return NotImplemented here, or raise an error.

Sounds reasonable, I'll try that.

aaltay · 2018-03-02T01:49:43Z

sdks/python/apache_beam/transforms/window.py


  def __eq__(self, other):
-    return self.start == other.start and self.end == other.end
+    return type(self) == type(other) and \


Same comment about type mismatch.

Can we put the whole test in parens and avoid backslash line termination as recommended in PEP8? The problem with backslash line termination is that a whitespace character to the right of the backslash breaks the script but is invisible to the reader.

aaltay · 2018-03-02T01:51:36Z

sdks/python/apache_beam/transforms/window.py

+  def __eq__(self, other):
+    return self.value == other.value and self.timestamp == other.timestamp
+
+  def __hash_(self):


Why do you need to implement this? Also there is a typo in the name __hash_.

I guess its not needed then :) I'll remove it.

aaltay · 2018-03-02T01:58:08Z

sdks/python/apache_beam/utils/windowed_value.py

    """
    if type(left) is not type(right):
-      return cmp(type(left), type(right))
+      return 1


This is a change in behaviour even though it matches the comment above. What does this affect?

All of the tests pass. If you were to sort a windowed value with something which was not a windowed value the ordering could change.

Sounds good.

cc: @robertwb since this pertains to the todo below.

aaltay · 2018-03-03T22:24:05Z

@holdenk Is this ready to review again? Github comment threading is confusing, I cannot always tell if all comments received a response or not.

holdenk · 2018-03-05T09:00:31Z

I need to fix the multi-line ''s I think. I'm busy dealing with some internal company stuff (it turns out I like getting paid) and this week is Strata SJ so I'll try and update this Tuesday night.

holdenk · 2018-03-07T18:30:45Z

ok cc @aaltay

cclauss

LGTM

holdenk · 2018-03-13T18:46:39Z

cc @aaltay & @robertwb

aaltay · 2018-03-13T20:38:02Z

@holdenk I still see that datastore helper methods are being deprecated. I thought we decided not to do that. Also there is an open question to you in test_stream.py. Could you address those?

holdenk · 2018-03-14T19:06:28Z

So the open "test_stream" issue, you pinged someone else on it for comment? Or is there another one?

holdenk · 2018-03-14T19:07:09Z

Removed the deprecation as well, sorry about that. cc @aaltay

aaltay · 2018-03-15T22:56:41Z

I can merge it once tests are green. For some reason, tests were not completed yet after a day.

aaltay · 2018-03-15T22:57:01Z

retest this please

…ed as performance important so please take a look, but this should be strictly a perf improvement.

…and it encourages cmp like behaviour.

…sn't seem to be used.

holdenk · 2018-04-02T20:17:07Z

CC @aaltay

aaltay · 2018-04-03T05:17:22Z

@holdenk do you know why tests are failing before we merge?

cclauss · 2018-04-03T08:17:16Z

There seems to be a failure installing cython==0.26.1... The current cython on PyPI is 0.28.1

aaltay · 2018-04-03T16:45:02Z

@cclauss perhaps it is a flake with installation? 0.26.1 is also available on pypi even though it is not the latest version.

cgarciae · 2018-04-04T02:43:33Z

Also getting this error :(
I am reverting to python 2 for this but hope it gets fixed soon.

stale · 2018-06-07T04:01:21Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.

stale · 2018-06-14T04:53:52Z

This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

aaltay reviewed Mar 2, 2018

View reviewed changes

cclauss approved these changes Mar 7, 2018

View reviewed changes

holdenk force-pushed the BEAM-3761-fix-py3-missing-functions branch from 8d36869 to 89f757a Compare March 13, 2018 18:51

aaltay approved these changes Mar 15, 2018

View reviewed changes

holdenk added 9 commits April 2, 2018 12:51

Change windowed value to not use cmp anymore. Note: this file as mark…

16beed9

…ed as performance important so please take a look, but this should be strictly a perf improvement.

Deprecate the comparators since I didn't find an active use for them …

d190bc2

…and it encourages cmp like behaviour.

Remove dead _variant_comparator test code which used cmp since it doe…

979e540

…sn't seem to be used.

Rewrite cmp methods to use eq/ne/lt and total_ordering

14b74b1

No longer compare windows of different types.

d1419e9

Style

aeb66a6

Remove __hash_ function.

2880bb2

Switch \ to ( for multi-line

0bdca44

Remove deprecations from the helper.

cd98dae

holdenk force-pushed the BEAM-3761-fix-py3-missing-functions branch from 43b9c66 to cd98dae Compare April 2, 2018 20:16

cclauss mentioned this pull request Jun 1, 2018

[WIP][BEAM-2784] Run python 2 to 3 migration and fix resulting Python 2 errors #3772

Closed

stale bot added the wontfix label Jun 7, 2018

stale bot closed this Jun 14, 2018

kennknowles added stale and removed wontfix labels Jun 25, 2018

cclauss mentioned this pull request Jun 30, 2018

[BEAM-3761] Define cmp() in Python 3 #5843

Merged

2 tasks

[BEAM-3761]Fix Python 3 cmp usage #4774

[BEAM-3761]Fix Python 3 cmp usage #4774

Uh oh!

Conversation

holdenk commented Feb 28, 2018

Uh oh!

holdenk commented Feb 28, 2018

Uh oh!

holdenk commented Mar 1, 2018

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!

cclauss Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

cclauss Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

holdenk Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaltay commented Mar 3, 2018

Uh oh!

holdenk commented Mar 5, 2018

Uh oh!

holdenk commented Mar 7, 2018

Uh oh!

cclauss left a comment

Choose a reason for hiding this comment

Uh oh!

holdenk commented Mar 13, 2018

Uh oh!

aaltay commented Mar 13, 2018

Uh oh!

holdenk commented Mar 14, 2018

Uh oh!

holdenk commented Mar 14, 2018

Uh oh!

aaltay commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cclauss Mar 2, 2018 •

edited

Loading

cclauss Mar 2, 2018 •

edited

Loading

holdenk Mar 2, 2018 •

edited

Loading

aaltay commented Mar 15, 2018 •

edited

Loading

cclauss commented Apr 3, 2018 •

edited

Loading