-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[BEAM-3444] Fix python3 flake8 errors e999 #4376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -147,14 +147,18 @@ def compute_term_frequency(uri_count_and_total): | |
| # | ||
| # This calculation uses a side input, a Dataflow-computed auxiliary value | ||
| # presented to each invocation of our MapFn lambda. The second argument to | ||
| # the lambda (called total---note that we are unpacking the first argument) | ||
| # the function (called total---note that the first argument is a tuple) | ||
| # receives the value we listed after the lambda in Map(). Additional side | ||
| # inputs (and ordinary Python values, too) can be provided to MapFns and | ||
| # DoFns in this way. | ||
| def div_word_count_by_total(word_count, total): | ||
| (word, count) = word_count | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional: You don't need parentheses in the unpacking or the packing into tuples. |
||
| return (word, float(count) / total) | ||
|
|
||
| word_to_df = ( | ||
| word_to_doc_count | ||
| | 'ComputeDocFrequencies' >> beam.Map( | ||
| lambda (word, count), total: (word, float(count) / total), | ||
| div_word_count_by_total, | ||
| AsSingleton(total_documents))) | ||
|
|
||
| # Join the term frequency and document frequency collections, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,8 @@ | |
| import sys | ||
| import traceback | ||
|
|
||
| import six | ||
|
|
||
| from apache_beam.internal import util | ||
| from apache_beam.metrics.execution import ScopedMetricsContainer | ||
| from apache_beam.pvalue import TaggedOutput | ||
|
|
@@ -512,7 +514,7 @@ def _reraise_augmented(self, exn): | |
| traceback.format_exception_only(type(exn), exn)[-1].strip() | ||
| + step_annotation) | ||
| new_exn._tagged_with_step = True | ||
| raise new_exn, None, original_traceback | ||
| six.raise_from(new_exn, original_traceback) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This use of Are the other uses of |
||
|
|
||
|
|
||
| class OutputProcessor(object): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,8 @@ | |
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| # | ||
| from __future__ import print_function | ||
|
|
||
| import functools | ||
| import logging | ||
| import time | ||
|
|
@@ -218,7 +220,7 @@ def test_progress_metrics(self): | |
| m_out.processed_elements.measured.output_element_counts['twice']) | ||
|
|
||
| except: | ||
| print res._metrics_by_stage | ||
| print(res._metrics_by_stage) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's use logging.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure since this is done in a test, I think printing might actually be the right behaviour.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, printing is fine. It's just so that if this test fails, there is some context. |
||
| raise | ||
|
|
||
| # Inherits all tests from maptask_executor_runner.MapTaskExecutorRunner | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ | |
| from concurrent import futures | ||
|
|
||
| import grpc | ||
| import six | ||
|
|
||
| from apache_beam.portability.api import beam_fn_api_pb2 | ||
| from apache_beam.portability.api import beam_fn_api_pb2_grpc | ||
|
|
@@ -50,7 +51,7 @@ def call_fn(): | |
| thread.join(timeout_secs) | ||
| if exc_info: | ||
| t, v, tb = exc_info # pylint: disable=unbalanced-tuple-unpacking | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we still need
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure, either way its probably unrelated to the flake8 changes. I think doing a follow up sweep of the linter disable statements could be a good starter task for someone later? |
||
| raise t, v, tb | ||
| six.reraise(t, v, tb) | ||
| assert not thread.is_alive(), 'timed out after %s seconds' % timeout_secs | ||
| return wrapper | ||
| return decorate | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| #!/bin/bash | ||
| # | ||
| # Licensed to the Apache Software Foundation (ASF) under one or more | ||
| # contributor license agreements. See the NOTICE file distributed with | ||
| # this work for additional information regarding copyright ownership. | ||
| # The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| # (the "License"); you may not use this file except in compliance with | ||
| # the License. You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| # | ||
|
|
||
| # This script will run python3 ready style checks | ||
| # | ||
| # Currently only flake8 E999 | ||
| # | ||
| # The exit-code of the script indicates success or a failure. | ||
|
|
||
| set -o errexit | ||
| set -o pipefail | ||
|
|
||
| MODULE=apache_beam | ||
|
|
||
| usage(){ echo "Usage: $0 [MODULE|--help] # The default MODULE is $MODULE"; } | ||
|
|
||
| if test $# -gt 0; then | ||
| case "$@" in | ||
| --help) usage; exit 1;; | ||
| *) MODULE="$*";; | ||
| esac | ||
| fi | ||
|
|
||
| echo "Running flake8 for module $MODULE:" | ||
| flake8 $MODULE --count --select=E999 --show-source --statistics | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need a separate file? I see the we are running the same thing in run_pylint.sh already.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So many of the current linters don't pass when run in a Py3 env. We can copy them over 1 and a time.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. Optional: We don't need mini in the name. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,16 +62,20 @@ echo "Running pylint for module $MODULE:" | |
| pylint -j8 "$MODULE" --ignore-patterns="$FILES_TO_IGNORE" | ||
| echo "Running pycodestyle for module $MODULE:" | ||
| pycodestyle "$MODULE" --exclude="$FILES_TO_IGNORE" | ||
| echo "Running flake8 for module $MODULE:" | ||
| flake8 $MODULE --count --select=E999 --show-source --statistics | ||
| echo "Running isort for module $MODULE:" | ||
| # Skip files where isort is behaving weirdly | ||
| ISORT_EXCLUDED=( | ||
| "apiclient.py" | ||
| "avroio_test.py" | ||
| "datastore_wordcount.py" | ||
| "datastoreio_test.py" | ||
| "hadoopfilesystem.py" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need exceptions for a growing list of files?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just saw this (my username is udim). Was wondering why this file was added to this list? |
||
| "iobase_test.py" | ||
| "fast_coders_test.py" | ||
| "slow_coders_test.py" | ||
| "vcfio.py" | ||
| ) | ||
| SKIP_PARAM="" | ||
| for file in "${ISORT_EXCLUDED[@]}"; do | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,8 @@ | |
|
|
||
| [tox] | ||
| # new environments will be excluded by default unless explicitly added to envlist. | ||
| envlist = py27,py27gcp,py27cython,lint,docs | ||
| # TODO (after BEAM-3671) add lint_py3 back in. | ||
| envlist = py27,py27gcp,py27cython,lint_py2,docs | ||
| toxworkdir = {toxinidir}/target/.tox | ||
|
|
||
| [pycodestyle] | ||
|
|
@@ -90,13 +91,14 @@ commands = | |
| python setup.py test | ||
| passenv = TRAVIS* | ||
|
|
||
| [testenv:lint] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was the name changed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I'll ping you on the PR for that. |
||
| [testenv:lint_py2] | ||
| deps= | ||
| nose==1.3.7 | ||
| pycodestyle==2.3.1 | ||
| pylint==1.7.2 | ||
| future==0.16.0 | ||
| isort==4.2.15 | ||
| flake8==3.5.0 | ||
| whitelist_externals=time | ||
| commands = | ||
| python --version | ||
|
|
@@ -105,6 +107,21 @@ commands = | |
| time {toxinidir}/run_pylint.sh | ||
| passenv = TRAVIS* | ||
|
|
||
| [testenv:lint_py3] | ||
| deps= | ||
| nose==1.3.7 | ||
| pycodestyle==2.3.1 | ||
| pylint==1.7.2 | ||
| future==0.16.0 | ||
| isort==4.2.15 | ||
| flake8==3.5.0 | ||
| whitelist_externals=time | ||
| commands = | ||
| time pip install -e .[test] | ||
| time {toxinidir}/run_mini_py3lint.sh | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need two lint environments? We run this command in the single lint environment.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need a new one for Python3 linting
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be clear flake8 only catches the E999 issues when run in Python3 |
||
| passenv = TRAVIS* | ||
|
|
||
|
|
||
| [testenv:docs] | ||
| deps= | ||
| nose==1.3.7 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment block is referring to the removed lambda.