[SPARK-6186] [EC2] Make Tachyon version configurable in EC2 deployment script #4901

ghost · 2015-03-05T07:14:54Z

This PR comes from Tachyon community to solve the issue:
https://tachyon.atlassian.net/browse/TACHYON-11

An accompanying PR is in mesos/spark-ec2:
mesos/spark-ec2#101

ghost · 2015-03-05T07:15:06Z

@haoyuan @shivaram

AmplabJenkins · 2015-03-05T07:17:10Z

Can one of the admins verify this patch?

srowen · 2015-03-05T10:50:28Z

Please make a JIRA

srowen · 2015-03-05T10:50:32Z

Ok to test

ghost · 2015-03-05T11:23:44Z

@srowen https://issues.apache.org/jira/browse/SPARK-6186

srowen · 2015-03-05T11:23:48Z

@uronce-cc you need to update the title of this PR. See https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark
It's minor, but I'd like to make sure people are following the process.

nchammas · 2015-03-05T16:06:58Z

ec2/spark_ec2.py

@@ -871,10 +898,11 @@ def deploy_files(conn, root_dir, opts, master_nodes, slave_nodes, modules):

    if "." in opts.spark_version:
        # Pre-built Spark deploy
-        spark_v = get_validate_spark_version(opts.spark_version, opts.spark_git_repo)
+        (spark_v, tachyon_v) = get_spark_tachyon_version(opts)


Why are we removing the validation of the Spark version here? And why are we ignoring the user-specified GitHub repo?

This change doesn't look correct to me. Did I misunderstand something?

nchammas · 2015-03-05T16:28:23Z

I don't think we should duplicate the list of valid Spark versions. Instead, it probably makes more sense to have a separate dictionary just for Tachyon version mappings and put that towards the beginning of the script as a global so it's easy to find and update, since that will be done manually.

Also, let's please reuse the existing code to validate the Spark version and avoid method signatures that just take an opts blob as an argument. I know we have them all over the place in spark-ec2, but we should move away from that.

For example:

SPARK_TACHYON_MAP = {
    "1.0.0": "0.4.1",
    "1.0.1": "0.4.1",
    "1.0.2": "0.4.1",
    "1.1.0": "0.5.0",
    "1.1.1": "0.5.0",
    "1.2.0": "0.5.0",
    "1.2.1": "0.5.0",
}

def get_tachyon_version(spark_version):
    if spark_version not in SPARK_TACHYON_MAP:
        return ""
    else:
        return SPARK_TACHYON_MAP[spark_version]

if "." in opts.spark_version:
    spark_v = get_validate_spark_version(opts.spark_version, opts.spark_git_repo)
    tachyon_v = get_tachyon_version(spark_v)
else:
   ...

shaneknapp · 2015-03-05T17:33:51Z

jenkins, test this please

SparkQA · 2015-03-05T17:39:13Z

Test build #28300 has started for PR 4901 at commit 66f66e5.

This patch merges cleanly.

SparkQA · 2015-03-05T17:40:27Z

Test build #28300 has finished for PR 4901 at commit 66f66e5.

This patch fails Python style tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-03-05T17:40:28Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28300/
Test FAILed.

ghost · 2015-03-06T01:44:26Z

@nchammas comments resolved. thanks.

srowen · 2015-03-06T10:34:30Z

@uronce-cc this apparently still does not pass Python style checks. Please have a look at the Jenkins output and/or run locally.

nchammas · 2015-03-06T18:13:50Z

FYI @haoyuan @uronce-cc: TACHYON-11 is not publicly visible.

nchammas · 2015-03-06T18:21:52Z

ec2/spark_ec2.py

@@ -148,6 +160,10 @@ def parse_args():
        "-v", "--spark-version", default=DEFAULT_SPARK_VERSION,
        help="Version of Spark to use: 'X.Y.Z' or a specific git hash (default: %default)")
    parser.add_option(
+        "--tachyon-version",
+        default=DEFAULT_TACHYON_VERSION,
+        help="When --spark-version is a git hash, this will be used as the version of Tachyon")


The wording here is a little confusing, I think. So this option only takes effect if --spark-version is a git hash and otherwise it is ignored, right?

And what's the purpose of the default value here? Doesn't seem to ever come into play.

ghost · 2015-03-07T01:27:37Z

@srowen pep8 resolved.
@nchammas re-word help message, DEFAULT_TACHYON_VERSION removed.

srowen · 2015-03-07T12:59:49Z

Jenkins, retest this please.

SparkQA · 2015-03-07T13:03:04Z

Test build #28366 has started for PR 4901 at commit df054cd.

This patch merges cleanly.

SparkQA · 2015-03-07T13:04:03Z

Test build #28366 has finished for PR 4901 at commit df054cd.

This patch fails Python style tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-03-07T13:04:04Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28366/
Test FAILed.

improve pep8 error when lacking argument pep8 again...

ghost · 2015-03-07T13:33:05Z

@srowen please test again... still pep8 problem.

srowen · 2015-03-07T13:34:35Z

@uronce-cc I thought you had run the checks locally per above and resolved them. Please do that. Rerunning jenkins would not solve it.

ghost · 2015-03-07T13:41:23Z

@srowen Yes, I've run locally pep8 check through the pep8 util provided via pip install pep8. Here is current output:

21:29 cc@mbp ec2 pep8 spark_ec2.py
spark_ec2.py:110:1: E402 module level import not at top of file
spark_ec2.py:111:1: E402 module level import not at top of file
spark_ec2.py:112:1: E402 module level import not at top of file
spark_ec2.py:125:9: W503 line break before binary operator
spark_ec2.py:700:9: W503 line break before binary operator
spark_ec2.py:701:9: W503 line break before binary operator
spark_ec2.py:1105:101: E501 line too long (102 > 100 characters)

current warnings/errors are not caused by my modification. Should I resolve all of the remaining errors or let them go.

srowen · 2015-03-07T13:47:43Z

@uronce-cc oh, no you should run ./dev/lint-python which runs the pep8 rules customized for this project, that Jenkins will run.

ghost · 2015-03-07T13:56:27Z

@srowen ok, current PR has passed pep8 by lint-python

srowen · 2015-03-07T14:30:24Z

I don't see that you pushed anything more to this branch?

ghost · 2015-03-07T15:49:51Z

@srowen I've stashed all subsequent commits to make the commit history clean.
The last pep8 error has been resolved:
https://github.com/uronce-cc/spark/blob/master/ec2/spark_ec2.py#L898-900

shivaram · 2015-03-07T16:02:36Z

It looks like @uronce-cc has pushed a new commit https://github.com/uronce-cc/spark/commit/6f8887e96d5a5aae488048038e05a0f573326b79 that hasn't been tested yet. (The older one which was tested was df054cd)

I'll kick off another Jenkins run

shivaram · 2015-03-07T16:02:42Z

Jenkins, retest this please

SparkQA · 2015-03-07T16:07:44Z

Test build #28368 has started for PR 4901 at commit 6f8887e.

This patch merges cleanly.

srowen · 2015-03-07T16:19:49Z

Oh weird, github still says the commit is 2 days old. Jenkins didn't retest automatically either. It's in progress now, yes. You don't need to worry about squashing as the merge script will do that.

ghost · 2015-03-07T16:43:57Z

could it be merged? another task is blocked by this. thanks.

SparkQA · 2015-03-07T17:27:51Z

Test build #28368 has finished for PR 4901 at commit 6f8887e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-03-07T17:27:55Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28368/
Test PASSed.

nchammas · 2015-03-07T19:07:32Z

ec2/spark_ec2.py

    else:
        # Spark-only custom deploy
        spark_v = "%s|%s" % (opts.spark_git_repo, opts.spark_version)
+        if opts.tachyon_version is None:


I'll let @shivaram chime in here, but this is a new restriction. Now a user cannot deploy Spark at a specific git hash anymore without also specifying a Tachyon hash.

I'm not sure that makes sense from the outset, but it also breaks users who have been using spark-ec2 just to deploy Spark.

Yeah this seems like an unnecessary restriction for users who just want to change the Spark git hash.

Given that users who use the git hash are going to be advanced users why don't we just skip version checks for it ? If users want to use a specific git hash I think its fine to expect them to figure out which tachyon version is required or not etc.

@shivaram You mean for advanced users, let them manually specify tachyon_v for themselves?

or export TACHYON_VERSION=xxx manually in bash before running spark_ec2, but then we have to disable export TACHYON_VERSION=xx in spark_ec2.py.

I think this option is appropriate, first, for users who do not use git hash to deploy, nothing has changed in the CLI, second, for those use git hash, this option is handy, just need to update document.

What I meant is that we could fall back to some default tachyon version (say 0.6) if spark git hash is set but tachyon version is not set. We don't need to do force the user to pass this option then but if the user needs some specific tachyon version or git-hash they can set it.

This can be done by just setting a default value for that flag.

In former commit, I do set a default value, but I think if someone uses git hash and doesn't specify tachyon version, and the default tachyon version doesn't match spark's, then it's hard for he/she to know the error unless he/she has carefully read the document or the command line help or read the src, so I removed it.

@shivaram share some thought?

I see your point, but as @nchammas mentioned only some fraction of Spark usage scenarios involve using Tachyon. If one just wants to say run the master branch to do some spark-perf benchmarks expecting the user to set a Tachyon version seems like a unnecessary burden.

If we don't want to use a default tachyon version I'd vote for just not starting tachyon in cases where a spark git hash is used. (i.e. remove tachyon from the list of modules and print a info message)

ghost · 2015-03-09T07:28:35Z

@shivaram I've removed tachyon when using git hash.

shivaram · 2015-03-09T08:31:56Z

Jenkins, retest this please

SparkQA · 2015-03-09T08:33:07Z

Test build #28385 has started for PR 4901 at commit fd2a48e.

This patch merges cleanly.

SparkQA · 2015-03-09T09:54:43Z

Test build #28385 has finished for PR 4901 at commit fd2a48e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-03-09T09:54:48Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28385/
Test PASSed.

shivaram · 2015-03-09T18:17:21Z

ec2/spark_ec2.py

    else:
        # Spark-only custom deploy
        spark_v = "%s|%s" % (opts.spark_git_repo, opts.spark_version)
+        tachyon_v = ""
+        print "Deploy spark via git hash, Tachyon won't be set up"


Deploy spark -> Deploying Spark

shivaram · 2015-03-09T18:18:00Z

Thanks @uronce-cc - Change looks good to me but for the minor comment inline.
@nchammas -- Any other comments ?

nchammas · 2015-03-09T22:15:30Z

So it looks like now we won't allow the Tachyon git hash to be specified if the Spark hash is specified. Fine by me.

LGTM pending @shivaram's inline comment.

ghost · 2015-03-09T23:41:38Z

@shivaram @nchammas thanks for your comments. Have resolved shivaram's last comment.

ghost · 2015-03-09T23:42:50Z

@shivaram Could you create branch-1.4 in mesos/spark-ec2 for my following PR to finally make Tachyon configurable? thanks.

ghost changed the title ~~make tachyon version configurable in ec2~~ [SPARK-6186] [EC2] Make Tachyon version configurable in EC2 deployment script Mar 5, 2015

nchammas reviewed Mar 5, 2015
View reviewed changes

ghost mentioned this pull request Mar 6, 2015

update tachyon version mesos/spark-ec2#102

Merged

nchammas reviewed Mar 6, 2015
View reviewed changes

make tachyon version configurable

6f8887e

improve pep8 error when lacking argument pep8 again...

nchammas reviewed Mar 7, 2015
View reviewed changes

cheng chang added 2 commits March 8, 2015 12:16

add default value to --tachyon-version

1d53c5c

Remove Tachyon when deploying through git hash

fd2a48e

shivaram reviewed Mar 9, 2015
View reviewed changes

minor re-wording

313aa36

asfgit closed this in 7c7d2d5 Mar 10, 2015

[SPARK-6186] [EC2] Make Tachyon version configurable in EC2 deployment script #4901

[SPARK-6186] [EC2] Make Tachyon version configurable in EC2 deployment script #4901

Conversation

ghost commented Mar 5, 2015

ghost commented Mar 5, 2015

AmplabJenkins commented Mar 5, 2015

srowen commented Mar 5, 2015

srowen commented Mar 5, 2015

ghost commented Mar 5, 2015

srowen commented Mar 5, 2015

Choose a reason for hiding this comment

nchammas commented Mar 5, 2015

shaneknapp commented Mar 5, 2015

SparkQA commented Mar 5, 2015

SparkQA commented Mar 5, 2015

AmplabJenkins commented Mar 5, 2015

ghost commented Mar 6, 2015

srowen commented Mar 6, 2015

nchammas commented Mar 6, 2015

Choose a reason for hiding this comment

ghost commented Mar 7, 2015

srowen commented Mar 7, 2015

SparkQA commented Mar 7, 2015

SparkQA commented Mar 7, 2015

AmplabJenkins commented Mar 7, 2015

ghost commented Mar 7, 2015

srowen commented Mar 7, 2015

ghost commented Mar 7, 2015

srowen commented Mar 7, 2015

ghost commented Mar 7, 2015

srowen commented Mar 7, 2015

ghost commented Mar 7, 2015

shivaram commented Mar 7, 2015

shivaram commented Mar 7, 2015

SparkQA commented Mar 7, 2015

srowen commented Mar 7, 2015

ghost commented Mar 7, 2015

SparkQA commented Mar 7, 2015

AmplabJenkins commented Mar 7, 2015

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

ghost commented Mar 9, 2015

shivaram commented Mar 9, 2015

SparkQA commented Mar 9, 2015

SparkQA commented Mar 9, 2015

AmplabJenkins commented Mar 9, 2015

Choose a reason for hiding this comment

shivaram commented Mar 9, 2015

nchammas commented Mar 9, 2015

ghost commented Mar 9, 2015

ghost commented Mar 9, 2015