You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@madlib.apache.org by GitBox <gi...@apache.org> on 2020/03/23 23:05:35 UTC
[GitHub] [madlib] kaknikhil opened a new pull request #491: DL: Fix disk
issue by using truncate guc
kaknikhil opened a new pull request #491: DL: Fix disk issue by using truncate guc
URL: https://github.com/apache/madlib/pull/491
While testing places10 with fit multiple (gpdb5 and gpdb6, 10 iterations and 20 msts), we ran out of disk space although we had at least 1.5T left at the beginning of the query.
Fixed this by using a gpdb (> 6.4) only guc that allows for releasing disk space inside a sub transaction by calling truncate (see commit message for more details)
Other changes (see individual commit for more details)
1. Utilities: Improve rename table logic
1. Graph: Add select statements to graph tests
1. DL: add tests for asserting that guc values are unchanged
JIRA: MADLIB-1406
- [ ] Add the module name, JIRA# to PR/commit and description.
- [ ] Add tests for the change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [madlib] orhankislal commented on a change in pull request #491:
DL: Fix disk issue by using truncate guc
Posted by GitBox <gi...@apache.org>.
orhankislal commented on a change in pull request #491: DL: Fix disk issue by using truncate guc
URL: https://github.com/apache/madlib/pull/491#discussion_r396829374
##########
File path: src/ports/postgres/modules/deep_learning/madlib_keras_fit_multiple_model.py_in
##########
@@ -484,7 +545,7 @@ class FitMultipleModel():
if self.validation_table:
self.update_info_table(mst, False)
- def run_training(self):
+ def run_training(self, mst_idx):
Review comment:
It looks like we don't use this new parameter inside the function.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [madlib] kaknikhil merged pull request #491: DL: Fix disk issue by
using truncate guc
Posted by GitBox <gi...@apache.org>.
kaknikhil merged pull request #491: DL: Fix disk issue by using truncate guc
URL: https://github.com/apache/madlib/pull/491
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [madlib] kaknikhil commented on a change in pull request #491: DL:
Fix disk issue by using truncate guc
Posted by GitBox <gi...@apache.org>.
kaknikhil commented on a change in pull request #491: DL: Fix disk issue by using truncate guc
URL: https://github.com/apache/madlib/pull/491#discussion_r396835254
##########
File path: src/ports/postgres/modules/deep_learning/test/madlib_keras_model_selection.sql_in
##########
@@ -210,6 +210,11 @@ SELECT assert(
'Keras Fit Multiple Output Summary Validation failed when user passes in 1-hot encoded label vector. Actual:' || __to_char(summary))
FROM (SELECT * FROM iris_multiple_model_summary) summary;
+-- Test the output table created are all persistent(not unlogged)
+SELECT assert(MADLIB_SCHEMA.is_table_unlogged('iris_multiple_model') = false, 'Model output table is unlogged');
Review comment:
we should add the same test for warm start as well
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [madlib] kaknikhil commented on a change in pull request #491: DL:
Fix disk issue by using truncate guc
Posted by GitBox <gi...@apache.org>.
kaknikhil commented on a change in pull request #491: DL: Fix disk issue by using truncate guc
URL: https://github.com/apache/madlib/pull/491#discussion_r396835067
##########
File path: src/ports/postgres/modules/deep_learning/madlib_keras_fit_multiple_model.py_in
##########
@@ -648,8 +651,9 @@ class FitMultipleModel():
with SetGUC("dev_opt_unsafe_truncate_in_subtransaction", "on"):
temp_model_table = unique_string('updated_model')
+ self.unlogged_table = self.unlogged_table if not self.is_final_training_call else ''
Review comment:
```suggestion
unlogged_table = self.unlogged_table if not self.is_final_training_call else ''
plpy.execute("""CREATE {unlogged_table} TABLE {temp_model_table} ( LIKE {self.model_output_table}""")
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [madlib] kaknikhil commented on a change in pull request #491: DL:
Fix disk issue by using truncate guc
Posted by GitBox <gi...@apache.org>.
kaknikhil commented on a change in pull request #491: DL: Fix disk issue by using truncate guc
URL: https://github.com/apache/madlib/pull/491#discussion_r396854086
##########
File path: src/ports/postgres/modules/deep_learning/madlib_keras_fit_multiple_model.py_in
##########
@@ -185,13 +226,16 @@ class FitMultipleModel():
mst_row = [self.grand_schedule[dist_key][mst_idx]
for dist_key in self.dist_keys]
self.create_mst_schedule_table(mst_row)
+ self.is_final_training_call = (iter == self.num_iterations and mst_idx == total_msts-1)
if mst_idx == 0:
start_iteration = time.time()
- self.run_training()
+ self.run_training(mst_idx)
Review comment:
see https://github.com/apache/madlib/pull/491#discussion_r396854057
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [madlib] kaknikhil commented on a change in pull request #491: DL:
Fix disk issue by using truncate guc
Posted by GitBox <gi...@apache.org>.
kaknikhil commented on a change in pull request #491: DL: Fix disk issue by using truncate guc
URL: https://github.com/apache/madlib/pull/491#discussion_r396854057
##########
File path: src/ports/postgres/modules/deep_learning/madlib_keras_fit_multiple_model.py_in
##########
@@ -484,7 +545,7 @@ class FitMultipleModel():
if self.validation_table:
self.update_info_table(mst, False)
- def run_training(self):
+ def run_training(self, mst_idx):
Review comment:
mst_idx has been added because it helps us print per hop time when we need to debug the fit multiple query. We might also add a verbose flag in the future to print per hop time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [madlib] orhankislal commented on a change in pull request #491:
DL: Fix disk issue by using truncate guc
Posted by GitBox <gi...@apache.org>.
orhankislal commented on a change in pull request #491: DL: Fix disk issue by using truncate guc
URL: https://github.com/apache/madlib/pull/491#discussion_r396829826
##########
File path: src/ports/postgres/modules/deep_learning/madlib_keras_fit_multiple_model.py_in
##########
@@ -185,13 +226,16 @@ class FitMultipleModel():
mst_row = [self.grand_schedule[dist_key][mst_idx]
for dist_key in self.dist_keys]
self.create_mst_schedule_table(mst_row)
+ self.is_final_training_call = (iter == self.num_iterations and mst_idx == total_msts-1)
if mst_idx == 0:
start_iteration = time.time()
- self.run_training()
+ self.run_training(mst_idx)
Review comment:
Same question as to the function definition. Is `mst_idx` being used during `run_training`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [madlib] fmcquillan99 commented on a change in pull request #491:
DL: Fix disk issue by using truncate guc
Posted by GitBox <gi...@apache.org>.
fmcquillan99 commented on a change in pull request #491: DL: Fix disk issue by using truncate guc
URL: https://github.com/apache/madlib/pull/491#discussion_r397417842
##########
File path: src/ports/postgres/modules/deep_learning/madlib_keras_fit_multiple_model.sql_in
##########
@@ -88,6 +88,10 @@ You can set up the models and hyperparameters to try with the
Model Selection</a> utility to define the unique combinations
of model architectures, compile and fit parameters.
+@note If madlib_keras_fit_multiple_model is running on gpdb5, the database will
+keep on adding to the disk space(depending on the model size) and will only
+release the disk space once the fit multiple query has completed execution.
+
Review comment:
Suggested slight changes:
If 'madlib_keras_fit_multiple_model()' is running on GPDB 5, the database will
keep adding to the disk space (in proportion to model size) and will only
release the disk space once the fit multiple query has completed execution.
This is not the case for GPDB 6+ where disk space is released during the
fit multiple query.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [madlib] kaknikhil commented on a change in pull request #491: DL:
Fix disk issue by using truncate guc
Posted by GitBox <gi...@apache.org>.
kaknikhil commented on a change in pull request #491: DL: Fix disk issue by using truncate guc
URL: https://github.com/apache/madlib/pull/491#discussion_r397422819
##########
File path: src/ports/postgres/modules/deep_learning/madlib_keras_fit_multiple_model.sql_in
##########
@@ -88,6 +88,10 @@ You can set up the models and hyperparameters to try with the
Model Selection</a> utility to define the unique combinations
of model architectures, compile and fit parameters.
+@note If madlib_keras_fit_multiple_model is running on gpdb5, the database will
+keep on adding to the disk space(depending on the model size) and will only
+release the disk space once the fit multiple query has completed execution.
+
Review comment:
sounds good
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [madlib] fmcquillan99 commented on a change in pull request #491:
DL: Fix disk issue by using truncate guc
Posted by GitBox <gi...@apache.org>.
fmcquillan99 commented on a change in pull request #491: DL: Fix disk issue by using truncate guc
URL: https://github.com/apache/madlib/pull/491#discussion_r397417842
##########
File path: src/ports/postgres/modules/deep_learning/madlib_keras_fit_multiple_model.sql_in
##########
@@ -88,6 +88,10 @@ You can set up the models and hyperparameters to try with the
Model Selection</a> utility to define the unique combinations
of model architectures, compile and fit parameters.
+@note If madlib_keras_fit_multiple_model is running on gpdb5, the database will
+keep on adding to the disk space(depending on the model size) and will only
+release the disk space once the fit multiple query has completed execution.
+
Review comment:
Suggested slight changes:
@note If 'madlib_keras_fit_multiple_model()' is running on GPDB 5, the database will
keep adding to the disk space (in proportion to model size) and will only
release the disk space once the fit multiple query has completed execution.
This is not the case for GPDB 6+ where disk space is released during the
fit multiple query.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [madlib] kaknikhil commented on a change in pull request #491: DL:
Fix disk issue by using truncate guc
Posted by GitBox <gi...@apache.org>.
kaknikhil commented on a change in pull request #491: DL: Fix disk issue by using truncate guc
URL: https://github.com/apache/madlib/pull/491#discussion_r396854116
##########
File path: src/ports/postgres/modules/utilities/utilities.sql_in
##########
@@ -541,3 +541,49 @@ BEGIN
RETURN 0;
END;
$$ LANGUAGE plpgsql;
+
+CREATE OR REPLACE FUNCTION MADLIB_SCHEMA.is_ver_greater_than_gp_640_or_pg_11()
Review comment:
makes sense. will do
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [madlib] orhankislal commented on a change in pull request #491:
DL: Fix disk issue by using truncate guc
Posted by GitBox <gi...@apache.org>.
orhankislal commented on a change in pull request #491: DL: Fix disk issue by using truncate guc
URL: https://github.com/apache/madlib/pull/491#discussion_r396830491
##########
File path: src/ports/postgres/modules/utilities/utilities.sql_in
##########
@@ -541,3 +541,49 @@ BEGIN
RETURN 0;
END;
$$ LANGUAGE plpgsql;
+
+CREATE OR REPLACE FUNCTION MADLIB_SCHEMA.is_ver_greater_than_gp_640_or_pg_11()
Review comment:
I understand why we need this very specific function but it would be great to add a few lines of comments for context. It is especially confusing because the function is in utilities and I usually expect them to be somewhat generic. Please note that I am not suggesting to move the function to DL, just some comments.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services