[BEAM-11516] Upgrade to pylint 2.11.1, fix warnings #15612

benWize · 2021-09-28T23:07:48Z

Please add a meaningful description for your change here

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
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.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

`ValidatesRunner` compliance status (on master branch)

Lang	ULR	Twister2
Go	---	---
Java
Python	---	---
XLang		---

Examples testing status on various runners

Lang	ULR	Dataflow	Flink	Samza	Spark	Twister2
Go	---	---	---	---	---	---	---
Java	---		---	---	---	---	---
Python	---	---	---	---	---	---	---
XLang	---	---	---	---	---	---	---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go	Java	Python

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---			---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

codecov · 2021-09-29T00:57:12Z

Codecov Report

Merging #15612 (80aec5c) into master (e525449) will increase coverage by 0.03%.
The diff coverage is 85.31%.

@@            Coverage Diff             @@
##           master   #15612      +/-   ##
==========================================
+ Coverage   83.76%   83.80%   +0.03%     
==========================================
  Files         444      444              
  Lines       60418    60462      +44     
==========================================
+ Hits        50612    50668      +56     
+ Misses       9806     9794      -12

Impacted Files	Coverage Δ
sdks/python/apache_beam/dataframe/partitionings.py	`96.03% <ø> (ø)`
sdks/python/apache_beam/dataframe/transforms.py	`95.18% <ø> (ø)`
sdks/python/apache_beam/examples/avro_bitcoin.py	`0.00% <ø> (ø)`
...thon/apache_beam/examples/complete/autocomplete.py	`62.85% <ø> (ø)`
...n/apache_beam/examples/complete/game/game_stats.py	`45.74% <ø> (ø)`
...e_beam/examples/complete/game/hourly_team_score.py	`59.49% <ø> (ø)`
...apache_beam/examples/complete/game/leader_board.py	`52.63% <ø> (ø)`
...n/apache_beam/examples/complete/game/user_score.py	`62.79% <ø> (ø)`
...e_beam/examples/complete/top_wikipedia_sessions.py	`80.70% <ø> (ø)`
...che_beam/examples/flink/flink_streaming_impulse.py	`0.00% <0.00%> (ø)`
... and 133 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c564f8...80aec5c. Read the comment docs.

benWize · 2021-09-29T23:33:12Z

Run Python 3.7 PostCommit

benWize · 2021-09-30T14:55:11Z

Hi @TheNeuralBit, would you help me to review this?

TheNeuralBit

Thank you this looks great! I looked over ~100 of the 237 files. Just to summarize the code changes for posterity:

super calls have all changed to super().__init__ (super-with-arguments)
re-raised errors include from e (raise-missing-from)
dict() -> {} (use-dict-literal)
check dictionary membership directly rather than using x in dict.keys() (consider-iterating-dictionary)
avoid creating intermediate list when creating a tuple (tuple([..]) -> tuple()) (not sure what pylint code this is)
Change imports like import apache_beam.transforms.window as window to from apache_beam.transforms import window

For reference: https://pylint.pycqa.org/en/latest/technical_reference/features.html

These all look like positive changes that shouldn't have any user-visible impact, except for the fixes for raise-missing-from. In many places we already have logic to inform the user about the underlying error, so it's redundant to use the from syntax as well. In general I think the from syntax is preferable, but we need to look over the exceptions to dedupe this.

Given the extreme risk of merge conflicts (this PR touches the entire code base), I'd recommend we back out the changes to the exceptions and file a follow-up jira to figure out what to do there (might be worth raising on dev@ too). That way we can go ahead and get the non-controversial changes merged before conflicts arise.

TheNeuralBit · 2021-09-30T16:15:05Z

sdks/python/apache_beam/utils/retry.py

              # Re-raise the original exception since we finished the retries.
-              raise exn.with_traceback(exn_traceback)


Here it looks intentional to not include stop_iteration. Could you disable the check instead?

TheNeuralBit · 2021-09-30T16:20:20Z

sdks/python/apache_beam/utils/profiler.py

@@ -23,7 +23,7 @@
 # pytype: skip-file
 # mypy: check-untyped-defs

-import cProfile  # pylint: disable=bad-python3-import
+import cProfile  # pylint: disable=bad-option-value


What is this for?

I removed it

TheNeuralBit · 2021-09-30T16:23:19Z

sdks/python/apache_beam/utils/processes.py

      else:
        raise RuntimeError("Full trace: {}, \
           output of the failed child process {} "\
-          .format(traceback.format_exc(), error.output))
+          .format(traceback.format_exc(), error.output)) from error


Nearly all of the exceptions in this file already include logic to print the original traceback in the error message. That's not necessary now that we have from error. Could you remove the trackeback.format_exc() logic?

TheNeuralBit · 2021-09-30T16:27:29Z

sdks/python/apache_beam/coders/coder_impl.py

@@ -747,6 +747,7 @@ def encode_to_stream(self, value, out, nested):
  def decode_from_stream(self, in_, nested):
    # type: (create_InputStream, bool) -> IntervalWindow
    if not TYPE_CHECKING:
+      # pylint: disable=global-variable-not-assigned


I think this will disable the check for the entire block, not just the line. You might put at the end of the line or use disable-next instead (docs)

TheNeuralBit · 2021-09-30T16:36:56Z

sdks/python/apache_beam/dataframe/partitionings.py

@@ -195,7 +195,7 @@ def shuffled(seq):
      random.shuffle(seq)
      return seq

-    # pylint: disable=range-builtin-not-iterating
+    # pylint: disable=bad-option-value
    part = pd.Series(shuffled(range(len(df))), index=df.index) % num_partitions


What is ths issue here?

TheNeuralBit · 2021-09-30T16:42:17Z

sdks/python/apache_beam/io/aws/s3filesystem.py

@@ -279,7 +279,7 @@ def checksum(self, path):
    try:
      return s3io.S3IO(options=self._options).checksum(path)
    except Exception as e:  # pylint: disable=broad-except
-      raise BeamIOError("Checksum operation failed", {path: e})
+      raise BeamIOError("Checksum operation failed", {path: e}) from e


It looks like these exceptions are already doing something with the original exception. Could you verify if we actually need the from e or not? I'd like to avoid printing the culprit stacktrace twice. (Same goes for other BeamIOErrors you've changed)

TheNeuralBit · 2021-09-30T17:03:07Z

sdks/python/apache_beam/io/gcp/gcsfilesystem.py

@@ -328,6 +328,7 @@ def _delete_path(path):
      match_result = self.match([path_to_use])[0]
      statuses = gcsio.GcsIO().delete_batch(
          [m.path for m in match_result.metadata_list])
+      # pylint: disable=used-before-assignment


Is this a false-positive? I can't see what it's upset about.

I think it is a false-positive

TheNeuralBit · 2021-09-30T17:08:17Z

sdks/python/.pylintrc

@@ -143,6 +147,7 @@ disable =
  unnecessary-pass,
  unneeded-not,
  unsubscriptable-object,
+  unspecified-encoding,


Some of these may be useful to apply eventually. unspecified-encoding in particular we should probably fix. Since Beam runs on distributed worker environments that may have surprising default encodings, it's good for our code to be explicit about encodings.

We could add a TODO and file a follow-up jira if it's too much to take on in this PR though.

Jira BEAM-12992 created to follow up unspecified-encoding warning

…le some warnings

benWize · 2021-09-30T23:40:46Z

I created BEAM-12991 to follow up raise-missing-from warning and removed the related changes from this PR.

TheNeuralBit

Thanks for the quick turnaround! I want to send a message out to dev@ about this, but will try to merge today.

TheNeuralBit · 2021-10-04T15:23:03Z

dev@ thread

benWize · 2021-10-04T16:17:18Z

Thank you @TheNeuralBit!

* [BEAM-11516] Upgrade pylint 2.11.1, fix warnings * [BEAM-11516] Fix typo * [BEAM-11516] Fix error infering __class__ * [BEAM-11516] Rollback operations super style * [BEAM-11516] Fix style * [BEAM-11516] Rollback raise-missing-from fixed warnings * [BEAM-11516] Remove unused pylint disable comments, add TODOs to enable some warnings * [BEAM-11516] Remove from error leftover * [BEAM-11516] Fix pylint warnings

[BEAM-11516] Upgrade pylint 2.11.1, fix warnings

2583220

benWize changed the title ~~[BEAM-11516] Upgrade to pylint 2.11.1, fix warnings~~ [WIP][BEAM-11516] Upgrade to pylint 2.11.1, fix warnings Sep 28, 2021

[BEAM-11516] Fix typo

ec5cd7f

benWize added 3 commits September 29, 2021 10:27

[BEAM-11516] Fix error infering __class__

c91f670

[BEAM-11516] Rollback operations super style

4e8f255

[BEAM-11516] Fix style

7a403b0

benWize changed the title ~~[WIP][BEAM-11516] Upgrade to pylint 2.11.1, fix warnings~~ [BEAM-11516] Upgrade to pylint 2.11.1, fix warnings Sep 30, 2021

benWize marked this pull request as ready for review September 30, 2021 14:53

TheNeuralBit self-requested a review September 30, 2021 15:56

TheNeuralBit requested changes Sep 30, 2021

View reviewed changes

benWize added 5 commits September 30, 2021 16:11

[BEAM-11516] Rollback raise-missing-from fixed warnings

42ac3f2

[BEAM-11516] Remove unused pylint disable comments, add TODOs to enab…

0a776aa

…le some warnings

[BEAM-11516] Remove from error leftover

c323d98

Merge branch 'master' into BEAM-11516

aafcd6c

[BEAM-11516] Fix pylint warnings

80aec5c

TheNeuralBit approved these changes Oct 1, 2021

View reviewed changes

TheNeuralBit merged commit 8e217ea into apache:master Oct 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-11516] Upgrade to pylint 2.11.1, fix warnings #15612

[BEAM-11516] Upgrade to pylint 2.11.1, fix warnings #15612

benWize commented Sep 28, 2021

codecov bot commented Sep 29, 2021 •

edited

Loading

benWize commented Sep 29, 2021

benWize commented Sep 30, 2021

TheNeuralBit left a comment

TheNeuralBit Sep 30, 2021

TheNeuralBit Sep 30, 2021

benWize Sep 30, 2021

TheNeuralBit Sep 30, 2021

TheNeuralBit Sep 30, 2021

benWize Sep 30, 2021

TheNeuralBit Sep 30, 2021

benWize Sep 30, 2021

TheNeuralBit Sep 30, 2021

TheNeuralBit Sep 30, 2021

benWize Sep 30, 2021

TheNeuralBit Sep 30, 2021

benWize Sep 30, 2021

benWize commented Sep 30, 2021

TheNeuralBit left a comment

TheNeuralBit commented Oct 4, 2021

benWize commented Oct 4, 2021

		# Re-raise the original exception since we finished the retries.
		raise exn.with_traceback(exn_traceback)

[BEAM-11516] Upgrade to pylint 2.11.1, fix warnings #15612

[BEAM-11516] Upgrade to pylint 2.11.1, fix warnings #15612

Conversation

benWize commented Sep 28, 2021

ValidatesRunner compliance status (on master branch)

Examples testing status on various runners

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

codecov bot commented Sep 29, 2021 • edited Loading

Codecov Report

benWize commented Sep 29, 2021

benWize commented Sep 30, 2021

TheNeuralBit 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

benWize commented Sep 30, 2021

TheNeuralBit left a comment

Choose a reason for hiding this comment

TheNeuralBit commented Oct 4, 2021

benWize commented Oct 4, 2021

`ValidatesRunner` compliance status (on master branch)

codecov bot commented Sep 29, 2021 •

edited

Loading