[SPARK-2470] PEP8 fixes to PySpark #1505

nchammas · 2014-07-21T03:36:04Z

This pull request aims to resolve all outstanding PEP8 violations in PySpark.

merging upstream updates

This update gives launched EC2 instances descriptive names by using instance tags. Launched instances now show up in the EC2 console with these names. I used `format()` with named parameters, which I believe is the recommended practice for string formatting in Python, but which doesn’t seem to be used elsewhere in the script.

pulling upstream updates

Merge upstream updates

merging upstream updates

Security groups created by spark-ec2 do not prepend “spark-“ to the name. Since naming the instances themselves is new to spark-ec2, it’s better to change that pattern to match the existing naming pattern for the security groups, rather than the other way around.

Functions in Python should be preceded by 2 blank lines, not 1.

merge upstream updates

AmplabJenkins · 2014-07-21T03:36:52Z

Can one of the admins verify this patch?

rxin · 2014-07-21T04:57:59Z

@davies can you take a look?

davies · 2014-07-21T21:22:27Z

python/pyspark/statcounter.py

@@ -124,5 +125,5 @@ def sampleStdev(self):
        return math.sqrt(self.sampleVariance())

    def __repr__(self):
-        return "(count: %s, mean: %s, stdev: %s, max: %s, min: %s)" % (self.count(), self.mean(), self.stdev(), self.max(), self.min())
-
+        return "(count: %s, mean: %s, stdev: %s, max: %s, min: %s)" %


Since cloudpickle is a third-party module, we should mostly leave it alone. Discussion here: #1505 (diff)

nchammas · 2014-07-21T23:09:29Z

OK, I've fixed the problems you pointed out, reverted the changes to cloudpickle, and confirmed that python/run-tests passes.

davies · 2014-07-22T00:51:07Z

Thanks, LGTM.
cc @rxin

nchammas · 2014-07-22T01:51:39Z

Reynold, is there anything else we need to clean up before we can have pep8 checks become part of the CI cycle?

Also, it sounds like we want to make an exception for cloudpickle. So that needs to be made somewhere when we turn this on.

rxin · 2014-07-22T03:38:30Z

If we have fixed all the problems, let's do it. We should add pep8 check to the Jenkins scripts (in /dev/).

I'm not 100% positive whether our Jenkins instances have pep8 installed. Is there a way to run pep8 if it is not installed in the system by default?

rxin · 2014-07-22T03:39:07Z

Jenkins, test this please.

rxin · 2014-07-22T03:39:11Z

Jenkins, add to whitelist.

SparkQA · 2014-07-22T03:43:20Z

QA tests have started for PR 1505. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16941/consoleFull

nchammas · 2014-07-22T05:02:16Z

If we have to, we could probably somehow package pep8 and its dependencies as a standalone. It's doable but I think also a bit ugly and harder to update.

As for adding to the Jenkins scripts, shall I take a crack at that in a separate PR?

How are you thinking we approach that? There are at least a couple of things we could do, like:

update /dev/run-tests to include pep8 checks directly
update /dev/run-tests to call a pythonstyle script under /dev/

We should probably update step 4 under "Contributing Code" in the "Contributing to Spark Guide" to cover Python style as well.

Also, would it make sense to replace sbt/sbt scalastyle with a new check-style script in /dev/ that checks style for both Scala and Python code?

rxin · 2014-07-22T05:05:22Z

Let's definitely add the pep8 check in a separate PR. I'm waiting for Jenkins to come back positive before merging this pull request.

I think it'd make sense to have a linter script for scala (dev/lint-scala), and a linter script (dev/lint-python) for python so it is easier for devs to run. lint-scala can just call scalastyle, while lint-python calls pep8.

And then run-tests should call both. And yes, let's update the wiki once we have this checked in.

SparkQA · 2014-07-22T05:22:52Z

QA results for PR 1505:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16941/consoleFull

rxin · 2014-07-22T05:30:40Z

Merging this in master!

rxin · 2014-07-22T05:31:26Z

Now I've merged this, do you mind submitting a pep8 checker as part of Jenkins?

nchammas · 2014-07-22T16:04:26Z

do you mind submitting a pep8 checker as part of Jenkins?

Will do. I won't be able to work on this today, but I will open a separate PR for this this week hopefully. Do we need a separate JIRA issue for that btw?

rxin · 2014-07-22T18:16:38Z

A separate PR and JIRA would be better (since they are for different things). Thanks!

nchammas · 2014-07-22T19:50:57Z

I've created SPARK-2627 to track the Jenkins/CI part of this work. I'll post future updates there.

This pull request aims to resolve all outstanding PEP8 violations in PySpark. Author: Nicholas Chammas <[email protected]> Author: nchammas <[email protected]> Closes apache#1505 from nchammas/master and squashes the following commits: 98171af [Nicholas Chammas] [SPARK-2470] revert PEP 8 fixes to cloudpickle cba7768 [Nicholas Chammas] [SPARK-2470] wrap expression list in parentheses e178dbe [Nicholas Chammas] [SPARK-2470] style - change position of line break 9127d2b [Nicholas Chammas] [SPARK-2470] wrap expression lists in parentheses 22132a4 [Nicholas Chammas] [SPARK-2470] wrap conditionals in parentheses 24639bc [Nicholas Chammas] [SPARK-2470] fix whitespace for doctest 7d557b7 [Nicholas Chammas] [SPARK-2470] PEP8 fixes to tests.py 8f8e4c0 [Nicholas Chammas] [SPARK-2470] PEP8 fixes to storagelevel.py b3b96cf [Nicholas Chammas] [SPARK-2470] PEP8 fixes to statcounter.py d644477 [Nicholas Chammas] [SPARK-2470] PEP8 fixes to worker.py aa3a7b6 [Nicholas Chammas] [SPARK-2470] PEP8 fixes to sql.py 1916859 [Nicholas Chammas] [SPARK-2470] PEP8 fixes to shell.py 95d1d95 [Nicholas Chammas] [SPARK-2470] PEP8 fixes to serializers.py a0fec2e [Nicholas Chammas] [SPARK-2470] PEP8 fixes to mllib c85e1e5 [Nicholas Chammas] [SPARK-2470] PEP8 fixes to join.py d14f2f1 [Nicholas Chammas] [SPARK-2470] PEP8 fixes to __init__.py 81fcb20 [Nicholas Chammas] [SPARK-2470] PEP8 fixes to resultiterable.py 1bde265 [Nicholas Chammas] [SPARK-2470] PEP8 fixes to java_gateway.py 7fc849c [Nicholas Chammas] [SPARK-2470] PEP8 fixes to daemon.py ca2d28b [Nicholas Chammas] [SPARK-2470] PEP8 fixes to context.py f4e0039 [Nicholas Chammas] [SPARK-2470] PEP8 fixes to conf.py a6d5e4b [Nicholas Chammas] [SPARK-2470] PEP8 fixes to cloudpickle.py f0a7ebf [Nicholas Chammas] [SPARK-2470] PEP8 fixes to rddsampler.py 4dd148f [nchammas] Merge pull request apache#5 from apache/master f7e4581 [Nicholas Chammas] unrelated pep8 fix a36eed0 [Nicholas Chammas] name ec2 instances and security groups consistently de7292a [nchammas] Merge pull request apache#4 from apache/master 2e4fe00 [nchammas] Merge pull request apache#3 from apache/master 89fde08 [nchammas] Merge pull request apache#2 from apache/master 69f6e22 [Nicholas Chammas] PEP8 fixes 2627247 [Nicholas Chammas] broke up lines before they hit 100 chars 6544b7e [Nicholas Chammas] [SPARK-2065] give launched instances names 69da6cf [nchammas] Merge pull request apache#1 from apache/master

nchammas added 27 commits June 10, 2014 17:46

Merge pull request #1 from apache/master

69da6cf

merging upstream updates

broke up lines before they hit 100 chars

2627247

PEP8 fixes

69f6e22

Merge pull request #2 from apache/master

89fde08

pulling upstream updates

Merge pull request #3 from apache/master

2e4fe00

Merge upstream updates

Merge pull request #4 from apache/master

de7292a

merging upstream updates

unrelated pep8 fix

f7e4581

Functions in Python should be preceded by 2 blank lines, not 1.

Merge pull request #5 from apache/master

4dd148f

merge upstream updates

[SPARK-2470] PEP8 fixes to rddsampler.py

f0a7ebf

[SPARK-2470] PEP8 fixes to cloudpickle.py

a6d5e4b

[SPARK-2470] PEP8 fixes to conf.py

f4e0039

[SPARK-2470] PEP8 fixes to context.py

ca2d28b

[SPARK-2470] PEP8 fixes to daemon.py

7fc849c

[SPARK-2470] PEP8 fixes to java_gateway.py

1bde265

[SPARK-2470] PEP8 fixes to resultiterable.py

81fcb20

[SPARK-2470] PEP8 fixes to __init__.py

d14f2f1

[SPARK-2470] PEP8 fixes to join.py

c85e1e5

[SPARK-2470] PEP8 fixes to mllib

a0fec2e

[SPARK-2470] PEP8 fixes to serializers.py

95d1d95

[SPARK-2470] PEP8 fixes to shell.py

1916859

[SPARK-2470] PEP8 fixes to sql.py

aa3a7b6

[SPARK-2470] PEP8 fixes to worker.py

d644477

[SPARK-2470] PEP8 fixes to statcounter.py

b3b96cf

[SPARK-2470] PEP8 fixes to storagelevel.py

8f8e4c0

[SPARK-2470] PEP8 fixes to tests.py

7d557b7

davies reviewed Jul 21, 2014
View reviewed changes

nchammas added 6 commits July 21, 2014 18:34

[SPARK-2470] fix whitespace for doctest

24639bc

[SPARK-2470] wrap conditionals in parentheses

22132a4

[SPARK-2470] wrap expression lists in parentheses

9127d2b

[SPARK-2470] style - change position of line break

e178dbe

[SPARK-2470] wrap expression list in parentheses

cba7768

[SPARK-2470] revert PEP 8 fixes to cloudpickle

98171af

Since cloudpickle is a third-party module, we should mostly leave it alone. Discussion here: #1505 (diff)

asfgit closed this in 5d16d5b Jul 22, 2014

rxin mentioned this pull request Jul 23, 2014

PEP8 compliance #1540

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-2470] PEP8 fixes to PySpark #1505

[SPARK-2470] PEP8 fixes to PySpark #1505

nchammas commented Jul 21, 2014

AmplabJenkins commented Jul 21, 2014

rxin commented Jul 21, 2014

davies Jul 21, 2014

nchammas commented Jul 21, 2014

davies commented Jul 22, 2014

nchammas commented Jul 22, 2014

rxin commented Jul 22, 2014

rxin commented Jul 22, 2014

rxin commented Jul 22, 2014

SparkQA commented Jul 22, 2014

nchammas commented Jul 22, 2014

rxin commented Jul 22, 2014

SparkQA commented Jul 22, 2014

rxin commented Jul 22, 2014

rxin commented Jul 22, 2014

nchammas commented Jul 22, 2014

rxin commented Jul 22, 2014

nchammas commented Jul 22, 2014

[SPARK-2470] PEP8 fixes to PySpark #1505

[SPARK-2470] PEP8 fixes to PySpark #1505

Conversation

nchammas commented Jul 21, 2014

AmplabJenkins commented Jul 21, 2014

rxin commented Jul 21, 2014

davies Jul 21, 2014

Choose a reason for hiding this comment

nchammas commented Jul 21, 2014

davies commented Jul 22, 2014

nchammas commented Jul 22, 2014

rxin commented Jul 22, 2014

rxin commented Jul 22, 2014

rxin commented Jul 22, 2014

SparkQA commented Jul 22, 2014

nchammas commented Jul 22, 2014

rxin commented Jul 22, 2014

SparkQA commented Jul 22, 2014

rxin commented Jul 22, 2014

rxin commented Jul 22, 2014

nchammas commented Jul 22, 2014

rxin commented Jul 22, 2014

nchammas commented Jul 22, 2014