Presto: More robust handling of existing config #109
There are no files selected for viewing
|
Contributor
There was a problem hiding this comment. lsmem may not be available on all environments. Consider adding a fallback (e.g., /proc/meminfo or free -g) so the script works across more hosts.
Contributor
Author
There was a problem hiding this comment. @patdevinwilson do you have an example of a system where |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,35 +17,36 @@ | |
| set -euo pipefail | ||
|
|
||
| RED='\033[0;31m' | ||
| YELLOW='\033[1;33m' | ||
| GREEN='\033[0;32m' | ||
| NC='\033[0m' # No Color | ||
|
|
||
| function echo_error { | ||
| echo -e "${RED}$1${NC}" | ||
| exit 1 | ||
| } | ||
|
|
||
| function echo_warning { | ||
| echo -e "${YELLOW}$1${NC}" | ||
| } | ||
|
|
||
| function echo_success { | ||
| echo -e "${GREEN}$1${NC}" | ||
| } | ||
|
|
||
| if [ ! -x ../pbench/pbench ]; then | ||
|
Contributor
There was a problem hiding this comment. Great little check, I'm wondering if we need to broadly apply such checks to all our scripts! (I realize this was not added in this PR but it just gets highlighted, and I think I've previously missed this.)
Contributor
Author
There was a problem hiding this comment. @misiugodfrey perhaps you can implement something equivalent in your script utilities stuff, that can be called from any script to verify that it's being run from the right location. |
||
| echo_error "ERROR: generate_presto_config.sh script must only be run from presto:presto/scripts" | ||
| fi | ||
|
|
||
| # get host values | ||
| NPROC=`nproc` | ||
| # lsmem will report in SI. Make sure we get values in GB. | ||
| RAM_GB=$(lsmem -b | grep "Total online memory" | awk '{print int($4 / (1024*1024*1024)); }') | ||
|
|
||
| # variant-specific behavior | ||
| # for GPU you must set vcpu_per_worker to a small number, not the CPU count | ||
| if [[ -z ${VARIANT_TYPE} || ! ${VARIANT_TYPE} =~ ^(cpu|gpu|java)$ ]]; then | ||
| echo_error "ERROR: VARIANT_TYPE must be set to a valid variant type (cpu, gpu, java)." | ||
| fi | ||
| if [[ "${VARIANT_TYPE}" == "gpu" ]]; then | ||
| VCPU_PER_WORKER=2 | ||
|
|
@@ -59,10 +60,16 @@ pushd ../docker/config > /dev/null | |
| # always move back even on failure | ||
| trap "popd > /dev/null" EXIT | ||
|
|
||
| CONFIG_DIR=generated/${VARIANT_TYPE} | ||
|
|
||
| # generate only if no existing config or overwrite flag is set | ||
|
Contributor
There was a problem hiding this comment. What configurations are being modified locally? Can you please expand on where the request for the updates in the PR is coming from?
Contributor
Author
There was a problem hiding this comment. @paul-aiyedun Todd and Patrick were both iterating configs by editing their local files. Once the original auto-config PR (#48) landed, this was dangerous, because the config would be automatically re-generated every time you ran the |
||
| if [[ ! -d ${CONFIG_DIR} || "${OVERWRITE_CONFIG}" == "true" ]]; then | ||
| echo "Generating Presto Config files for '${VARIANT_TYPE}' for host with ${NPROC} CPU cores and ${RAM_GB}GB RAM" | ||
|
|
||
| # (re-)generate the config.json file | ||
| rm -rf ${CONFIG_DIR} | ||
|
Contributor
There was a problem hiding this comment. Does this need special handling in case the script fails to complete (i.e. do we need a
Contributor
Author
There was a problem hiding this comment. Do you mean it should remove any config files it manages to generate if it doesn't generate it PROPERLY? At this point, the only thing that can fail is |
||
| mkdir -p ${CONFIG_DIR} | ||
| cat > ${CONFIG_DIR}/config.json << EOF | ||
| { | ||
| "cluster_size": "small", | ||
| "coordinator_instance_type": "${NPROC}-core CPU and ${RAM_GB}GB RAM", | ||
|
|
@@ -77,19 +84,24 @@ cat > generated/config.json << EOF | |
| } | ||
| EOF | ||
|
|
||
| # run pbench to generate the config files | ||
| # hide default pbench logging which goes to stderr so we only see any errors | ||
| if ../../pbench/pbench genconfig -p params.json -t template ${CONFIG_DIR} 2>&1 | grep '\{\"level":"error"'; then | ||
| echo_error "ERROR: Errors reported by pbench genconfig. Configs were not generated successfully." | ||
| fi | ||
|
|
||
| # now perform other variant-specific modifications to the generated configs | ||
| if [[ "${VARIANT_TYPE}" == "gpu" ]]; then | ||
| # for GPU variant, uncomment these optimizer settings | ||
|
Contributor
There was a problem hiding this comment. (Also not added in this PR but just being brought to my attention in this one) "uncomment these ... when" seems like a perfect fit for an CLI option that enables this (optionally of course!)
Contributor
Author
There was a problem hiding this comment. Yeah, this was #111. I don't think this should be a command-line option, as it MUST be enabled for GPU, and MUST NOT be enabled for CPU. This was my latest way of avoiding forking the template files, which @paul-aiyedun and others are opposed to. |
||
| # optimizer.joins-not-null-inference-strategy=USE_FUNCTION_METADATA | ||
| # optimizer.default-filter-factor-enabled=true | ||
| COORD_CONFIG="${CONFIG_DIR}/etc_coordinator/config_native.properties" | ||
| sed -i 's/\#optimizer/optimizer/g' ${COORD_CONFIG} | ||
| fi | ||
|
|
||
| # success message | ||
| echo_success "Configs were generated successfully" | ||
| else | ||
| # otherwise, reuse existing config | ||
| echo_success "Reusing existing Presto Config files for '${VARIANT_TYPE}'" | ||
| fi | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each of the three variants now has a separate generated config tree, so ALL of the mappings have to move to the leaf container recipes.