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

cclauss · 2018-06-30T08:32:51Z

Signed-off-by: cclauss cclauss@bluewin.ch

Please add a meaningful description for your change here
Define cmp() function that was removed in Python 3. A more straight ahead approach to #4774 that should be easier to review. Fixes the following issues:

flake8 testing of https://github.com/apache/beam on Python 3.6.3

$ flake8 . --count --select=E901,E999,F821,F822,F823 --show-source --statistics

./sdks/python/apache_beam/io/vcfio_test.py:77:14: F821 undefined name 'cmp'
      return cmp(v1.end, v2.end)
             ^
./sdks/python/apache_beam/io/vcfio_test.py:78:12: F821 undefined name 'cmp'
    return cmp(v1.start, v2.start)
           ^
./sdks/python/apache_beam/io/vcfio_test.py:79:10: F821 undefined name 'cmp'
  return cmp(v1.reference_name, v2.reference_name)
         ^
./sdks/python/apache_beam/io/gcp/datastore/v1/helper.py:93:12: F821 undefined name 'cmp'
  result = cmp(p1.kind, p2.kind)
           ^
./sdks/python/apache_beam/io/gcp/datastore/v1/helper.py:101:12: F821 undefined name 'cmp'
    return cmp(p1.id, p2.id)
           ^
./sdks/python/apache_beam/io/gcp/datastore/v1/helper.py:106:10: F821 undefined name 'cmp'
  return cmp(p1.name, p2.name)
         ^
./sdks/python/apache_beam/transforms/window.py:195:12: F821 undefined name 'cmp'
    return cmp(self.end, other.end) or cmp(hash(self), hash(other))
           ^
./sdks/python/apache_beam/transforms/window.py:195:40: F821 undefined name 'cmp'
    return cmp(self.end, other.end) or cmp(hash(self), hash(other))
                                       ^
./sdks/python/apache_beam/transforms/window.py:250:14: F821 undefined name 'cmp'
      return cmp(type(self), type(other))
             ^
./sdks/python/apache_beam/transforms/window.py:251:12: F821 undefined name 'cmp'
    return cmp((self.value, self.timestamp), (other.value, other.timestamp))
           ^

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

Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang	Apex	Dataflow	Flink	Gearpump	Samza	Spark
Go	---	---	---	---	---	---
Java
Python	---		---	---	---	---

cclauss · 2018-06-30T08:33:27Z

@aaltay @holdenk

holdenk · 2018-06-30T15:00:15Z

So I’d still rather remove them long term, but I agree in the short term hopefully this is easier to get a committer to sign off on.

That being said having 3 copies of the function isn’t great, how about moving it somewhere common?

cclauss · 2018-06-30T15:45:56Z

Agreed. When I thought about centralizing it but the three files are not near each other in the hierarchy (io, testing, transforms) but I suppose we could do something like from apache_beam.utils.compat import cmp It also stuck me as a bit weird (in legacy Python) to import a builtin from another file so that made the try/except block essential which in turn means that just one line per file is actually saved. I will make the change if that seems like the best approach.

superbobry · 2018-06-30T21:17:47Z

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


 # pylint: enable=ungrouped-imports

+try:


This one-liner has the same effect:

from past.bulitins import cmp

@superbobry A perfect suggestion. Thanks much. Done.

holdenk

LGTM

pabloem · 2018-07-02T18:10:27Z

It seems that there were failures in Cython: https://builds.apache.org/job/beam_PreCommit_Python_Commit/166/console

pabloem · 2018-07-02T18:10:45Z

You can run this test locally via tox -e py27-cython from within sdks/python/

superbobry · 2018-07-02T23:06:16Z

I'm curious, why does Cython fail on Jenkins? I've tried locally with both Python 2.7 and 3.6 and Cython 0.28.1, and the following imports fine:

from past.builtins import unicode

print(unicode)

Could it be that py27-cython does not install future which is needed for the import to work? What do you think @pabloem?

holdenk · 2018-07-02T23:13:33Z

You could try updating tox.ini to explicitly mention that as a dep?

cclauss · 2018-07-02T23:18:22Z

The current approach is safer... Only redefine cmp() when it is necessary.

We only have 18 more months to get this done.

holdenk · 2018-07-02T23:25:38Z

No, I'm not suggesting you change your approach I'm talking about addressing the jenkins build issues?

cclauss · 2018-07-02T23:34:53Z

There are no Jenkins build issues using my approach ;-)

Besides, I think you already put futurize in all appropriate places in tox.ini

holdenk · 2018-07-02T23:43:44Z

Isn't https://builds.apache.org/job/beam_PreCommit_Python_Commit/166/console from your approach or am I looking at the wrong build logs?

pabloem · 2018-07-02T23:46:06Z

The build ran once more after d9d8689 was pushed. See: https://builds.apache.org/job/beam_PreCommit_Python_Commit/199/

cclauss · 2018-07-02T23:58:33Z

Yes. The commits were squashed into a single commit.

holdenk · 2018-07-03T00:40:25Z

Ah ok, sorry for my confusion then :)

superbobry · 2018-07-03T05:57:14Z

There are no Jenkins build issues using my approach ;-)

No, but your approach introduces boilerplate. I think this is worth fixing given that a) there seem to be no issue with importing from past on Cython, and b) a past import is no less safe than try-except NameError.

cclauss · 2018-07-03T06:05:17Z

OK. Can you please create a new PR that solves the problem and when it is checked in then I will gladly close this one. I have zero pride of ownership on these fixes... I merely want them resolved.

superbobry · 2018-07-03T06:50:41Z

Sure, will do! I actually have a PR open which addresses similar patterns in the code (see #5869).

As soon as apache#5843 and apache#5900 are closed, we should add F821 (undefined names) tests to the flake8 tests on Python 3. This should flag commits that contain unqualified past.builtin imports such as basestring, cmp, raw_input, reload, unicode, xrange, etc.

Signed-off-by: cclauss <cclauss@bluewin.ch>

cclauss · 2018-07-10T06:02:11Z

@charlesccychen Your review please?

charlesccychen · 2018-07-10T07:35:42Z

Run Python PostCommit

charlesccychen · 2018-07-10T07:37:03Z

Thanks! This LGTM. Will merge once tests pass.

cclauss · 2018-07-10T16:32:28Z

Can someone please help me to decipher what went wrong?

https://builds.apache.org/job/beam_PostCommit_Python_Verify_PR/23/

charlesccychen · 2018-07-10T16:36:49Z

Strange, looks like it crashed / timed out in the middle.

charlesccychen · 2018-07-10T16:37:01Z

Run Python SDK PostCommit

charlesccychen · 2018-07-10T16:46:27Z

Run Python Dataflow ValidatesRunner

cclauss · 2018-07-10T18:30:07Z

I am even more confused.

charlesccychen · 2018-07-10T18:38:43Z

This looks fine, and the ValidatesRunner tests pass, so I'm going to merge even though Jenkins seems to buggy right now for the PostSubmit run.

cclauss · 2018-07-10T18:44:59Z

Thanks all!

superbobry reviewed Jun 30, 2018

View reviewed changes

holdenk approved these changes Jul 1, 2018

View reviewed changes

cclauss force-pushed the Define-cmp-in-Python3 branch from 9c2fcbc to d9d8689 Compare July 2, 2018 21:54

cclauss force-pushed the Define-cmp-in-Python3 branch 2 times, most recently from 18741b5 to d318ec4 Compare July 9, 2018 05:19

cclauss mentioned this pull request Jul 9, 2018

[BEAM-3959] Add Python 3 undefined names to flake8 #5901

Closed

2 tasks

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

f9242ca

Signed-off-by: cclauss <cclauss@bluewin.ch>

cclauss force-pushed the Define-cmp-in-Python3 branch from 4b614e0 to f9242ca Compare July 10, 2018 05:36

[BEAM-3959] Add Python 3 undefined names to flake8

779558e

charlesccychen merged commit 4c81d4e into apache:master Jul 10, 2018

cclauss deleted the Define-cmp-in-Python3 branch July 10, 2018 18:40

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

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

Uh oh!

Conversation

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

Uh oh!

Post-Commit Tests Status (on master branch)

Uh oh!

cclauss commented Jun 30, 2018

Uh oh!

holdenk commented Jun 30, 2018

Uh oh!

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

Uh oh!

Uh oh!

superbobry Jun 30, 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!

cclauss Jun 30, 2018

Choose a reason for hiding this comment

Uh oh!

holdenk left a comment

Choose a reason for hiding this comment

Uh oh!

pabloem commented Jul 2, 2018

Uh oh!

pabloem commented Jul 2, 2018

Uh oh!

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

Uh oh!

Uh oh!

holdenk commented Jul 2, 2018

Uh oh!

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

Uh oh!

Uh oh!

holdenk commented Jul 2, 2018

Uh oh!

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

Uh oh!

Uh oh!

holdenk commented Jul 2, 2018

Uh oh!

pabloem commented Jul 2, 2018

Uh oh!

cclauss commented Jul 2, 2018

Uh oh!

holdenk commented Jul 3, 2018

Uh oh!

superbobry commented Jul 3, 2018

Uh oh!

cclauss commented Jul 3, 2018

Uh oh!

superbobry commented Jul 3, 2018

Uh oh!

cclauss commented Jul 10, 2018

Uh oh!

charlesccychen commented Jul 10, 2018

Uh oh!

charlesccychen commented Jul 10, 2018

Uh oh!

cclauss commented Jul 10, 2018

Uh oh!

charlesccychen commented Jul 10, 2018

Uh oh!

charlesccychen commented Jul 10, 2018

Uh oh!

charlesccychen commented Jul 10, 2018

Uh oh!

cclauss commented Jul 10, 2018

Uh oh!

charlesccychen commented Jul 10, 2018

Uh oh!

cclauss commented Jul 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

cclauss commented Jun 30, 2018 •

edited

Loading

cclauss commented Jun 30, 2018 •

edited

Loading

superbobry Jun 30, 2018 •

edited

Loading

superbobry commented Jul 2, 2018 •

edited

Loading

cclauss commented Jul 2, 2018 •

edited

Loading

cclauss commented Jul 2, 2018 •

edited

Loading