[
https://issues.apache.org/jira/browse/BEAM-11516?focusedWorklogId=658503&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-658503
]
ASF GitHub Bot logged work on BEAM-11516:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 30/Sep/21 17:14
Start Date: 30/Sep/21 17:14
Worklog Time Spent: 10m
Work Description: TheNeuralBit commented on a change in pull request
#15612:
URL: https://github.com/apache/beam/pull/15612#discussion_r719569457
##########
File path: sdks/python/apache_beam/utils/processes.py
##########
@@ -49,56 +49,59 @@ def call(*args, **kwargs):
kwargs['shell'] = True
try:
out = subprocess.call(*args, **kwargs)
- except OSError:
- raise RuntimeError("Executable {} not found".format(args[0]))
+ except OSError as os_error:
+ raise RuntimeError(
+ "Executable {} not found".format(args[0])) from os_error
except subprocess.CalledProcessError as error:
if isinstance(args, tuple) and (args[0][2] == "pip"):
raise RuntimeError( \
"Full traceback: {}\n Pip install failed for package: {} \
\n Output from execution of subprocess: {}" \
- .format(traceback.format_exc(), args[0][6], error. output))
+ .format(traceback.format_exc(), args[0][6], error. output)) from
error
else:
raise RuntimeError("Full trace: {}\
\n Output of the failed child process: {} " \
- .format(traceback.format_exc(), error.output))
+ .format(traceback.format_exc(), error.output)) from error
return out
def check_call(*args, **kwargs):
if force_shell:
kwargs['shell'] = True
try:
out = subprocess.check_call(*args, **kwargs)
- except OSError:
- raise RuntimeError("Executable {} not found".format(args[0]))
+ except OSError as os_error:
+ raise RuntimeError(
+ "Executable {} not found".format(args[0])) from os_error
except subprocess.CalledProcessError as error:
if isinstance(args, tuple) and (args[0][2] == "pip"):
raise RuntimeError( \
"Full traceback: {} \n Pip install failed for package: {} \
\n Output from execution of subprocess: {}" \
- .format(traceback.format_exc(), args[0][6], error.output))
+ .format(traceback.format_exc(), args[0][6], error.output)) from error
else:
raise RuntimeError("Full trace: {} \
\n Output of the failed child process: {}" \
- .format(traceback.format_exc(), error.output))
+ .format(traceback.format_exc(), error.output)) from error
return out
def check_output(*args, **kwargs):
if force_shell:
kwargs['shell'] = True
try:
out = subprocess.check_output(*args, **kwargs)
- except OSError:
- raise RuntimeError("Executable {} not found".format(args[0]))
+ except OSError as os_error:
+ raise RuntimeError(
+ "Executable {} not found".format(args[0])) from os_error
except subprocess.CalledProcessError as error:
if isinstance(args, tuple) and (args[0][2] == "pip"):
raise RuntimeError( \
"Full traceback: {} \n Pip install failed for package: {} \
\n Output from execution of subprocess: {}" \
- .format(traceback.format_exc(), args[0][6], error.output))
+ .format(traceback.format_exc(), args[0][6], error.output)) from error
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
Review comment:
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?
##########
File path: 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
Review comment:
What is this for?
##########
File path: 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
Review comment:
Is this a false-positive? I can't see what it's upset about.
##########
File path: 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
Review comment:
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)
##########
File path: sdks/python/apache_beam/utils/retry.py
##########
@@ -261,9 +261,9 @@ def wrapper(*args, **kwargs):
try:
try:
sleep_interval = next(retry_intervals)
- except StopIteration:
+ except StopIteration as stop_iteration:
# Re-raise the original exception since we finished the retries.
- raise exn.with_traceback(exn_traceback)
Review comment:
Here it looks intentional to not include `stop_iteration`. Could you
disable the check instead?
##########
File path: 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
Review comment:
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](https://pylint.pycqa.org/en/latest/faq.html#how-to-disable-a-particular-message))
##########
File path: sdks/python/.pylintrc
##########
@@ -143,6 +147,7 @@ disable =
unnecessary-pass,
unneeded-not,
unsubscriptable-object,
+ unspecified-encoding,
Review comment:
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.
##########
File path: 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
Review comment:
What is ths issue here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@beam.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Issue Time Tracking
-------------------
Worklog Id: (was: 658503)
Time Spent: 3.5h (was: 3h 20m)
> Upgrade to pylint >=2.6.0
> -------------------------
>
> Key: BEAM-11516
> URL: https://issues.apache.org/jira/browse/BEAM-11516
> Project: Beam
> Issue Type: Task
> Components: sdk-py-core, testing
> Reporter: Brian Hulette
> Assignee: Benjamin Gonzalez
> Priority: P3
> Time Spent: 3.5h
> Remaining Estimate: 0h
>
> I accidentally ran the latest pylint (2.6.0 as opposed to 2.4.3 which we
> currently use), and found that it raised a lot of warnings, many of which
> looked useful. For example:
> {code}
> apache_beam/pvalue.py:500:11: R1725: Consider using Python 3 style super()
> without arguments (super-with-arguments)
> apache_beam/utils/timestamp.py:132:6: W0707: Consider explicitly re-raising
> using the 'from' keyword (raise-missing-from)
> {code}
> We should address these warnings and run 2.6.0 (or whatever is the latest at
> time of implementation) continuously
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
