[SYSTEMML-1451][Phase 2] Decouple Scripts and HDFS support #575
Conversation
|
@nakul02 could you please share your thoughts on this initial approach. Also what do you think of the following points below: I have tested |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
@akchinSTC can you test this once again. Thanks. |
|
@j143 - @akchinSTC is a bot. It will run the CI tests again when the author of this PR pushes another commit. These scripts don't add integration tests, so running the CI will not tell you more than running the tests on the master branch. Any reason you wanted this tested again? |
|
Refer to this link for build results (access rights to CI server needed): |
|
yes! @nakul02 There was a fix by another PR before this, which is otherwise responsible for previous build failure https://sparktc.ibmcloud.com/jenkins/job/SystemML-PullRequestBuilder/1777/ . Now, that test is passing in that particular test. |
|
FullReblockTest is one of our flaky tests which unfortunately fails once in a while. |
|
@j143 @nakul02 Note that it is possible to manually trigger a Jenkins PR test (if ever needed). See https://gist.github.com/deroneriksson/e0d6d0634f3388f0df5e#pull-request-magic for the command. |
|
@deroneriksson - is there a way to disable it? This PR introduces a set of python scripts which are never invoked directly or indirectly in anything that Jenkins does. Running the tests over and over again for each commit seems wasteful. It would be good to be able to disable the automatic running of the jenkins for every commit. |
|
I don't know if there's a way to disable it. There are situations where that would be very nice. |
|
I asked around and "skip ci" may do it. skip ci |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
krishnakalyan3
force-pushed
the
SYSTEMML-1451-phase2
branch
from
fe378f6
to
7367005
Compare
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): Failed Tests: 1SystemML-PullRequestBuilder/org.apache.systemml:systemml: 1 |
|
Refer to this link for build results (access rights to CI server needed): |
bin/systemml-spark-submit.py
Outdated
| conf = default_conf | ||
| def spark_submit_entry(master, driver_memory, num_executors, executor_memory, | ||
| executor_cores, conf, | ||
| nvargs, args, config, explain, debug, stats, gpu, f): | ||
|
|
There was a problem hiding this comment.
Could you please add a little comment about this function.
bin/utils.py
Outdated
|
|
||
|
|
||
| def get_env(): | ||
| """ |
There was a problem hiding this comment.
Could you please give this function a more descriptive name. get_env() is too generic and doesn't convey that only SPARK_HOME and SYSTEMML_HOME variables are being returned.
I would suggest you break this function up into two functions - one to get the environment variable for SYSTEMML_HOME and one for SPARK_HOME. You can then name them get_env_systemml_home and get_env_spark_home or something better.
bin/systemml-spark-submit.py
Outdated
|
|
||
| cparser = argparse.ArgumentParser(description='System-ML Spark Submit Script') | ||
| # SPARK-SUBMIT Options | ||
| cparser.add_argument('--master', default='local[*]', help='local, yarn-client, yarn-cluster', metavar='') |
There was a problem hiding this comment.
Can you please also print out the defaults for each of the options in the help message?
bin/systemml-standalone.py
Outdated
| if len(sys.argv) < 2: | ||
| print('Wrong usage') | ||
| print_usage_and_exit() | ||
| def standalone_entry(nvargs, args, config, explain, debug, stats, gpu, f): | ||
|
|
There was a problem hiding this comment.
Please add documentation for this function.
Also, do you think standalone_execution_entry or standalone_mode_entry is a better name?
bin/utils.py
Outdated
|
|
||
| def find_file(name, path): | ||
| """ | ||
|
|
There was a problem hiding this comment.
Could you please complete this documentation?
| return return_data | ||
|
|
||
|
|
||
| def get_std_out(process): |
There was a problem hiding this comment.
This name is somewhat misleading, since you return both - stdout and stderr. Maybe you could rename it to something more appropriate?
| return out_arr, error_arr | ||
|
|
||
|
|
||
| def parse_dir(std_outs): |
There was a problem hiding this comment.
How about the function name parse_hdfs_paths instead of parse_dir?
scripts/perftest/python/utils_fs.py
Outdated
| os.makedirs(directory) | ||
|
|
||
|
|
||
| def write_success(time, cwd): |
There was a problem hiding this comment.
I thought cwd is mostly to specify the current working directory.
How about naming this variable to something else, like directory or dir or something similar?
scripts/perftest/python/utils_fs.py
Outdated
| open(full_path, 'w').close() | ||
|
|
||
|
|
||
| def get_existence(path): |
There was a problem hiding this comment.
Maybe the name could be check_SUCCESS_file_exists
| # This file contains all misc utility functions required by performance test module | ||
|
|
||
|
|
||
| def sup_args(config_dict, spark_dict, exec_type): |
There was a problem hiding this comment.
Not sure what this name means. Could you please think of a more descriptive name?
|
Thanks for the review @nakul02. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
LGTM, i shall merge. |
Completed these tasks as part for Phase 2 for Google Summer of Code '17 - Decouple systemml-spark-submit.py - Decouple systemml-standalone.py - Refractor perf test suit to accept args like debug, stats, config etc... - Add HDFS support - Google Docs support - Compare SystemML with previous versions - Pylint, Comment - Extra arguments configuration Test - Windows Test - Doc update - systemml standalone comments - systemml spark submit comments Closes #575
Completed these tasks as part for Phase 2 for Google Summer of Code '17 - Decouple systemml-spark-submit.py - Decouple systemml-standalone.py - Refractor perf test suit to accept args like debug, stats, config etc... - Add HDFS support - Google Docs support - Compare SystemML with previous versions - Pylint, Comment - Extra arguments configuration Test - Windows Test - Doc update - systemml standalone comments - systemml spark submit comments Closes apache#575
Please refer to https://issues.apache.org/jira/browse/SYSTEMML-1451 for more details.