From a8ccc5f2ebc1462a7e5037ea042301f3e5253818 Mon Sep 17 00:00:00 2001 From: David Monllao Date: Tue, 29 Aug 2017 17:45:15 +0200 Subject: [PATCH 1/4] MDL-59630 analytics: Move get_analysables to abstract class --- analytics/classes/local/analyser/base.php | 68 +++++++++++++++++-- .../classes/local/analyser/by_course.php | 64 +---------------- analytics/classes/local/analyser/sitewide.php | 32 ++------- analytics/classes/model.php | 41 +++++------ .../analytics/analyser/student_enrolments.php | 2 +- 5 files changed, 91 insertions(+), 116 deletions(-) diff --git a/analytics/classes/local/analyser/base.php b/analytics/classes/local/analyser/base.php index ab2f52ea796..63c75b5a788 100644 --- a/analytics/classes/local/analyser/base.php +++ b/analytics/classes/local/analyser/base.php @@ -112,6 +112,16 @@ abstract class base { $this->log = array(); } + /** + * Returns the list of analysable elements available on the site. + * + * \core_analytics\local\analyser\by_course and \core_analytics\local\analyser\sitewide are implementing + * this method returning site courses (by_course) and the whole system (sitewide) as analysables. + * + * @return \core_analytics\analysable[] + */ + abstract public function get_analysables(); + /** * This function returns this analysable list of samples. * @@ -141,7 +151,7 @@ abstract class base { * * @return string */ - abstract protected function get_samples_origin(); + abstract public function get_samples_origin(); /** * Returns the context of a sample. @@ -166,15 +176,29 @@ abstract class base { /** * Main analyser method which processes the site analysables. * - * \core_analytics\local\analyser\by_course and \core_analytics\local\analyser\sitewide are implementing - * this method returning site courses (by_course) and the whole system (sitewide) as analysables. - * In most of the cases you should have enough extending from one of these classes so you don't need - * to reimplement this method. - * * @param bool $includetarget * @return \stored_file[] */ - abstract public function get_analysable_data($includetarget); + public function get_analysable_data($includetarget) { + + $filesbytimesplitting = array(); + + $analysables = $this->get_analysables(); + foreach ($analysables as $analysable) { + + $files = $this->process_analysable($analysable, $includetarget); + + // Later we will need to aggregate data by time splitting method. + foreach ($files as $timesplittingid => $file) { + $filesbytimesplitting[$timesplittingid][$analysable->get_id()] = $file; + } + } + + // We join the datasets by time splitting method. + $timesplittingfiles = $this->merge_analysable_files($filesbytimesplitting, $includetarget); + + return $timesplittingfiles; + } /** * Samples data this analyser provides. @@ -220,6 +244,36 @@ abstract class base { } } + /** + * Merges analysable dataset files into 1. + * + * @param array $filesbytimesplitting + * @param bool $includetarget + * @return \stored_file[] + */ + protected function merge_analysable_files($filesbytimesplitting, $includetarget) { + + $timesplittingfiles = array(); + foreach ($filesbytimesplitting as $timesplittingid => $files) { + + if ($this->options['evaluation'] === true) { + // Delete the previous copy. Only when evaluating. + \core_analytics\dataset_manager::delete_previous_evaluation_file($this->modelid, $timesplittingid); + } + + // Merge all course files into one. + if ($includetarget) { + $filearea = \core_analytics\dataset_manager::LABELLED_FILEAREA; + } else { + $filearea = \core_analytics\dataset_manager::UNLABELLED_FILEAREA; + } + $timesplittingfiles[$timesplittingid] = \core_analytics\dataset_manager::merge_datasets($files, + $this->modelid, $timesplittingid, $filearea, $this->options['evaluation']); + } + + return $timesplittingfiles; + } + /** * Checks that this analyser satisfies the provided indicator requirements. * diff --git a/analytics/classes/local/analyser/by_course.php b/analytics/classes/local/analyser/by_course.php index e531bba4d93..8e5b266a1db 100644 --- a/analytics/classes/local/analyser/by_course.php +++ b/analytics/classes/local/analyser/by_course.php @@ -40,14 +40,14 @@ abstract class by_course extends base { * * @return \core_analytics\course[] */ - public function get_courses() { + public function get_analysables() { // Default to all system courses. if (!empty($this->options['filter'])) { $courses = $this->options['filter']; } else { // Iterate through all potentially valid courses. - $courses = get_courses(); + $courses = get_courses('all', 'c.sortorder ASC'); } unset($courses[SITEID]); @@ -55,7 +55,7 @@ abstract class by_course extends base { foreach ($courses as $course) { // Skip the frontpage course. $analysable = \core_analytics\course::instance($course); - $analysables[$analysable->get_id()] = $analysable; + $analysables[] = $analysable; } if (empty($analysables)) { @@ -64,62 +64,4 @@ abstract class by_course extends base { return $analysables; } - - /** - * Returns the analysed data - * - * @param bool $includetarget - * @return \stored_file[] - */ - public function get_analysable_data($includetarget) { - - $filesbytimesplitting = array(); - - // This class and all children will iterate through a list of courses (\core_analytics\course). - $analysables = $this->get_courses('all', 'c.sortorder ASC'); - foreach ($analysables as $analysableid => $analysable) { - - $files = $this->process_analysable($analysable, $includetarget); - - // Later we will need to aggregate data by time splitting method. - foreach ($files as $timesplittingid => $file) { - $filesbytimesplitting[$timesplittingid][$analysableid] = $file; - } - } - - // We join the datasets by time splitting method. - $timesplittingfiles = $this->merge_analysable_files($filesbytimesplitting, $includetarget); - - return $timesplittingfiles; - } - - /** - * Merges analysable dataset files into 1. - * - * @param array $filesbytimesplitting - * @param bool $includetarget - * @return \stored_file[] - */ - protected function merge_analysable_files($filesbytimesplitting, $includetarget) { - - $timesplittingfiles = array(); - foreach ($filesbytimesplitting as $timesplittingid => $files) { - - if ($this->options['evaluation'] === true) { - // Delete the previous copy. Only when evaluating. - \core_analytics\dataset_manager::delete_previous_evaluation_file($this->modelid, $timesplittingid); - } - - // Merge all course files into one. - if ($includetarget) { - $filearea = \core_analytics\dataset_manager::LABELLED_FILEAREA; - } else { - $filearea = \core_analytics\dataset_manager::UNLABELLED_FILEAREA; - } - $timesplittingfiles[$timesplittingid] = \core_analytics\dataset_manager::merge_datasets($files, - $this->modelid, $timesplittingid, $filearea, $this->options['evaluation']); - } - - return $timesplittingfiles; - } } diff --git a/analytics/classes/local/analyser/sitewide.php b/analytics/classes/local/analyser/sitewide.php index 9c6fb47eea0..2a21359141f 100644 --- a/analytics/classes/local/analyser/sitewide.php +++ b/analytics/classes/local/analyser/sitewide.php @@ -36,36 +36,12 @@ defined('MOODLE_INTERNAL') || die(); abstract class sitewide extends base { /** - * Returns the analysable data. + * Returns one single analysable element, the site. * - * @param bool $includetarget - * @return \stored_file[] One file for each time splitting method. + * @return \core_analytics\analysable[] */ - public function get_analysable_data($includetarget) { - - // Here there is a single analysable and it is the system. + public function get_analysables() { $analysable = new \core_analytics\site(); - - $files = $this->process_analysable($analysable, $includetarget); - - // Copy to range files as there is just one analysable. - foreach ($files as $timesplittingid => $file) { - - if ($this->options['evaluation'] === true) { - // Delete the previous copy. Only when evaluating. - \core_analytics\dataset_manager::delete_previous_evaluation_file($this->modelid, $timesplittingid); - } - - // We use merge but it is just a copy. - if ($includetarget) { - $filearea = \core_analytics\dataset_manager::LABELLED_FILEAREA; - } else { - $filearea = \core_analytics\dataset_manager::UNLABELLED_FILEAREA; - } - $files[$timesplittingid] = \core_analytics\dataset_manager::merge_datasets(array($file), $this->modelid, - $timesplittingid, $filearea, $this->options['evaluation']); - } - - return $files; + return array($analysable); } } diff --git a/analytics/classes/model.php b/analytics/classes/model.php index dbee08afb29..fc7f43d1af0 100644 --- a/analytics/classes/model.php +++ b/analytics/classes/model.php @@ -247,15 +247,15 @@ class model { /** * Returns the model analyser (defined by the model target). * + * @param array $options Default initialisation with no options. * @return \core_analytics\local\analyser\base */ - public function get_analyser() { + public function get_analyser($options = array()) { if ($this->analyser !== null) { return $this->analyser; } - // Default initialisation with no options. - $this->init_analyser(); + $this->init_analyser($options); return $this->analyser; } @@ -276,26 +276,29 @@ class model { throw new \moodle_exception('errornotarget', 'analytics'); } - if (!empty($options['evaluation'])) { - // The evaluation process will run using all available time splitting methods unless one is specified. - if (!empty($options['timesplitting'])) { - $timesplitting = \core_analytics\manager::get_time_splitting($options['timesplitting']); - $timesplittings = array($timesplitting->get_id() => $timesplitting); + $timesplittings = array(); + if (empty($options['notimesplitting'])) { + if (!empty($options['evaluation'])) { + // The evaluation process will run using all available time splitting methods unless one is specified. + if (!empty($options['timesplitting'])) { + $timesplitting = \core_analytics\manager::get_time_splitting($options['timesplitting']); + $timesplittings = array($timesplitting->get_id() => $timesplitting); + } else { + $timesplittings = \core_analytics\manager::get_enabled_time_splitting_methods(); + } } else { - $timesplittings = \core_analytics\manager::get_enabled_time_splitting_methods(); - } - } else { - if (empty($this->model->timesplitting)) { - throw new \moodle_exception('invalidtimesplitting', 'analytics', '', $this->model->id); + if (empty($this->model->timesplitting)) { + throw new \moodle_exception('invalidtimesplitting', 'analytics', '', $this->model->id); + } + + // Returned as an array as all actions (evaluation, training and prediction) go through the same process. + $timesplittings = array($this->model->timesplitting => $this->get_time_splitting()); } - // Returned as an array as all actions (evaluation, training and prediction) go through the same process. - $timesplittings = array($this->model->timesplitting => $this->get_time_splitting()); - } - - if (empty($timesplittings)) { - throw new \moodle_exception('errornotimesplittings', 'analytics'); + if (empty($timesplittings)) { + throw new \moodle_exception('errornotimesplittings', 'analytics'); + } } if (!empty($options['evaluation'])) { diff --git a/lib/classes/analytics/analyser/student_enrolments.php b/lib/classes/analytics/analyser/student_enrolments.php index 5d9a0d0cc2b..e084440e9e4 100644 --- a/lib/classes/analytics/analyser/student_enrolments.php +++ b/lib/classes/analytics/analyser/student_enrolments.php @@ -49,7 +49,7 @@ class student_enrolments extends \core_analytics\local\analyser\by_course { * * @return string */ - protected function get_samples_origin() { + public function get_samples_origin() { return 'user_enrolments'; } From 99b84a26f9daff9dfc35aa48ce8b9f4a952b4426 Mon Sep 17 00:00:00 2001 From: David Monllao Date: Wed, 30 Aug 2017 12:53:38 +0200 Subject: [PATCH 2/4] MDL-59630 analytics: Purge everything when the model is deleted --- .../classes/output/form/edit_model.php | 2 +- analytics/classes/model.php | 14 ++- analytics/classes/prediction.php | 2 +- analytics/tests/model_test.php | 106 ++++++++++++++++++ 4 files changed, 119 insertions(+), 5 deletions(-) diff --git a/admin/tool/analytics/classes/output/form/edit_model.php b/admin/tool/analytics/classes/output/form/edit_model.php index c8f71299409..66270b25cbe 100644 --- a/admin/tool/analytics/classes/output/form/edit_model.php +++ b/admin/tool/analytics/classes/output/form/edit_model.php @@ -45,7 +45,7 @@ class edit_model extends \moodleform { $mform = $this->_form; - if ($this->_customdata['model']->get_model_obj()->trained == 1) { + if ($this->_customdata['model']->is_trained()) { $message = get_string('edittrainedwarning', 'tool_analytics'); $mform->addElement('html', $OUTPUT->notification($message, \core\output\notification::NOTIFY_WARNING)); } diff --git a/analytics/classes/model.php b/analytics/classes/model.php index fc7f43d1af0..d8b1ab2216f 100644 --- a/analytics/classes/model.php +++ b/analytics/classes/model.php @@ -441,9 +441,6 @@ class model { // Delete generated predictions. $this->clear_model(); - // Purge all generated files. - \core_analytics\dataset_manager::clear_model_files($this->model->id); - // Reset trained flag. $this->model->trained = 0; @@ -476,6 +473,7 @@ class model { $this->clear_model(); $DB->delete_records('analytics_models', array('id' => $this->model->id)); + $DB->delete_records('analytics_models_log', array('modelid' => $this->model->id)); } /** @@ -1413,11 +1411,21 @@ class model { private function clear_model() { global $DB; + $predictionids = $DB->get_fieldset_select('analytics_predictions', 'id', 'modelid = :modelid', + array('modelid' => $this->get_id())); + if ($predictionids) { + list($sql, $params) = $DB->get_in_or_equal($predictionids); + $DB->delete_records_select('analytics_prediction_actions', "predictionid $sql", $params); + } + $DB->delete_records('analytics_predictions', array('modelid' => $this->model->id)); $DB->delete_records('analytics_predict_samples', array('modelid' => $this->model->id)); $DB->delete_records('analytics_train_samples', array('modelid' => $this->model->id)); $DB->delete_records('analytics_used_files', array('modelid' => $this->model->id)); + // Purge all generated files. + \core_analytics\dataset_manager::clear_model_files($this->model->id); + // We don't expect people to clear models regularly and the cost of filling the cache is // 1 db read per context. $this->purge_insights_cache(); diff --git a/analytics/classes/prediction.php b/analytics/classes/prediction.php index 549795e816b..24fa77a12e7 100644 --- a/analytics/classes/prediction.php +++ b/analytics/classes/prediction.php @@ -68,7 +68,7 @@ class prediction { /** * Constructor * - * @param \stdClass $prediction + * @param \stdClass|int $prediction * @param array $sampledata * @return void */ diff --git a/analytics/tests/model_test.php b/analytics/tests/model_test.php index b8e1768ded8..48bd6300762 100644 --- a/analytics/tests/model_test.php +++ b/analytics/tests/model_test.php @@ -77,6 +77,93 @@ class analytics_model_testcase extends advanced_testcase { $this->assertInstanceOf('\core_analytics\model', $model); } + /** + * test_delete + */ + public function test_delete() { + global $DB; + + $this->resetAfterTest(true); + set_config('enabled_stores', 'logstore_standard', 'tool_log'); + + $coursepredict1 = $this->getDataGenerator()->create_course(array('visible' => 0)); + $coursepredict2 = $this->getDataGenerator()->create_course(array('visible' => 0)); + $coursetrain1 = $this->getDataGenerator()->create_course(array('visible' => 1)); + $coursetrain2 = $this->getDataGenerator()->create_course(array('visible' => 1)); + + $this->model->enable('\core\analytics\time_splitting\no_splitting'); + + $this->model->train(); + $this->model->predict(); + + // Fake evaluation results record to check that it is actually deleted. + $this->add_fake_log(); + + // Generate a prediction action to confirm that it is deleted when there is an important update. + $predictions = $DB->get_records('analytics_predictions'); + $prediction = reset($predictions); + $prediction = new \core_analytics\prediction($prediction, array('whatever' => 'not used')); + $prediction->action_executed(\core_analytics\prediction::ACTION_FIXED, $this->model->get_target()); + + $this->model->delete(); + $this->assertEmpty($DB->count_records('analytics_models', array('id' => $this->modelobj->id))); + $this->assertEmpty($DB->count_records('analytics_models_log', array('modelid' => $this->modelobj->id))); + $this->assertEmpty($DB->count_records('analytics_predictions')); + $this->assertEmpty($DB->count_records('analytics_prediction_actions')); + $this->assertEmpty($DB->count_records('analytics_train_samples')); + $this->assertEmpty($DB->count_records('analytics_predict_samples')); + $this->assertEmpty($DB->count_records('analytics_used_files')); + + set_config('enabled_stores', '', 'tool_log'); + get_log_manager(true); + } + + /** + * test_clear + */ + public function test_clear() { + global $DB; + + $this->resetAfterTest(true); + set_config('enabled_stores', 'logstore_standard', 'tool_log'); + + $coursepredict1 = $this->getDataGenerator()->create_course(array('visible' => 0)); + $coursepredict2 = $this->getDataGenerator()->create_course(array('visible' => 0)); + $coursetrain1 = $this->getDataGenerator()->create_course(array('visible' => 1)); + $coursetrain2 = $this->getDataGenerator()->create_course(array('visible' => 1)); + + $this->model->enable('\core\analytics\time_splitting\no_splitting'); + + $this->model->train(); + $this->model->predict(); + + // Fake evaluation results record to check that it is actually deleted. + $this->add_fake_log(); + + // Generate a prediction action to confirm that it is deleted when there is an important update. + $predictions = $DB->get_records('analytics_predictions'); + $prediction = reset($predictions); + $prediction = new \core_analytics\prediction($prediction, array('whatever' => 'not used')); + $prediction->action_executed(\core_analytics\prediction::ACTION_FIXED, $this->model->get_target()); + + // Update to an empty time splitting method to force clear_model execution. + $this->model->update(1, false, ''); + // Restore previous time splitting method. + $this->model->enable('\core\analytics\time_splitting\no_splitting'); + + // Check that most of the stuff got deleted. + $this->assertEquals(1, $DB->count_records('analytics_models', array('id' => $this->modelobj->id))); + $this->assertEquals(1, $DB->count_records('analytics_models_log', array('modelid' => $this->modelobj->id))); + $this->assertEmpty($DB->count_records('analytics_predictions')); + $this->assertEmpty($DB->count_records('analytics_prediction_actions')); + $this->assertEmpty($DB->count_records('analytics_train_samples')); + $this->assertEmpty($DB->count_records('analytics_predict_samples')); + $this->assertEmpty($DB->count_records('analytics_used_files')); + + set_config('enabled_stores', '', 'tool_log'); + get_log_manager(true); + } + public function test_model_manager() { $this->resetAfterTest(true); @@ -159,6 +246,25 @@ class analytics_model_testcase extends advanced_testcase { $target = \core_analytics\manager::get_target('\core\analytics\target\no_teaching'); $this->assertTrue(\core_analytics\model::exists($target)); } + + /** + * Generates a model log record. + */ + private function add_fake_log() { + global $DB, $USER; + + $log = new stdClass(); + $log->modelid = $this->modelobj->id; + $log->version = $this->modelobj->version; + $log->target = $this->modelobj->target; + $log->indicators = $this->modelobj->indicators; + $log->score = 1; + $log->info = json_encode([]); + $log->dir = 'not important'; + $log->timecreated = time(); + $log->usermodified = $USER->id; + $DB->insert_record('analytics_models_log', $log); + } } /** From f9222c4953d352dc9a58522d9f91ac59d77db7e7 Mon Sep 17 00:00:00 2001 From: David Monllao Date: Wed, 30 Aug 2017 14:20:15 +0200 Subject: [PATCH 3/4] MDL-59630 analytics: New clean up task --- analytics/classes/manager.php | 48 ++++++ .../test_target_course_level_shortname.php | 46 ++++++ analytics/tests/manager_test.php | 153 ++++++++++++++++++ lang/en/admin.php | 1 + lib/classes/task/analytics_cleanup_task.php | 55 +++++++ lib/db/tasks.php | 9 ++ 6 files changed, 312 insertions(+) create mode 100644 analytics/tests/fixtures/test_target_course_level_shortname.php create mode 100644 analytics/tests/manager_test.php create mode 100644 lib/classes/task/analytics_cleanup_task.php diff --git a/analytics/classes/manager.php b/analytics/classes/manager.php index fff2c50e919..3ab5dd91f6c 100644 --- a/analytics/classes/manager.php +++ b/analytics/classes/manager.php @@ -489,6 +489,54 @@ class manager { } } + /** + * Cleans up analytics db tables that do not directly depend on analysables that may have been deleted. + */ + public static function cleanup() { + global $DB; + + // Clean up stuff that depends on contexts that do not exist anymore. + $sql = "SELECT DISTINCT ap.contextid FROM {analytics_predictions} ap + LEFT JOIN {context} ctx ON ap.contextid = ctx.id + WHERE ctx.id IS NULL"; + $apcontexts = $DB->get_records_sql($sql); + + $sql = "SELECT DISTINCT aic.contextid FROM {analytics_indicator_calc} aic + LEFT JOIN {context} ctx ON aic.contextid = ctx.id + WHERE ctx.id IS NULL"; + $indcalccontexts = $DB->get_records_sql($sql); + + $contexts = $apcontexts + $indcalccontexts; + if ($contexts) { + list($sql, $params) = $DB->get_in_or_equal(array_keys($contexts)); + $DB->execute("DELETE FROM {analytics_prediction_actions} apa WHERE apa.predictionid IN + (SELECT ap.id FROM {analytics_predictions} ap WHERE ap.contextid $sql)", $params); + + $DB->delete_records_select('analytics_predictions', "contextid $sql", $params); + $DB->delete_records_select('analytics_indicator_calc', "contextid $sql", $params); + } + + // Clean up stuff that depends on analysable ids that do not exist anymore. + $models = self::get_all_models(); + foreach ($models as $model) { + $analyser = $model->get_analyser(array('notimesplitting' => true)); + $analysables = $analyser->get_analysables(); + if (!$analysables) { + continue; + } + + $analysableids = array_map(function($analysable) { + return $analysable->get_id(); + }, $analysables); + + list($notinsql, $params) = $DB->get_in_or_equal($analysableids, SQL_PARAMS_NAMED, 'param', false); + $params['modelid'] = $model->get_id(); + + $DB->delete_records_select('analytics_predict_samples', "modelid = :modelid AND analysableid $notinsql", $params); + $DB->delete_records_select('analytics_train_samples', "modelid = :modelid AND analysableid $notinsql", $params); + } + } + /** * Returns the provided element classes in the site. * diff --git a/analytics/tests/fixtures/test_target_course_level_shortname.php b/analytics/tests/fixtures/test_target_course_level_shortname.php new file mode 100644 index 00000000000..1f13d463ac9 --- /dev/null +++ b/analytics/tests/fixtures/test_target_course_level_shortname.php @@ -0,0 +1,46 @@ +. + +/** + * Test target. + * + * @package core_analytics + * @copyright 2017 David Monllaó {@link http://www.davidmonllao.com} + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +require_once(__DIR__ . '/test_target_shortname.php'); + +/** + * Test target. + * + * @package core_analytics + * @copyright 2017 David Monllaó {@link http://www.davidmonllao.com} + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class test_target_course_level_shortname extends test_target_shortname { + + /** + * get_analyser_class + * + * @return string + */ + public function get_analyser_class() { + return '\core\analytics\analyser\courses'; + } +} diff --git a/analytics/tests/manager_test.php b/analytics/tests/manager_test.php new file mode 100644 index 00000000000..f4c2a1c06bf --- /dev/null +++ b/analytics/tests/manager_test.php @@ -0,0 +1,153 @@ +. + +/** + * Unit tests for the manager. + * + * @package core_analytics + * @copyright 2017 David Monllaó {@link http://www.davidmonllao.com} + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +require_once(__DIR__ . '/fixtures/test_indicator_max.php'); +require_once(__DIR__ . '/fixtures/test_indicator_min.php'); +require_once(__DIR__ . '/fixtures/test_indicator_fullname.php'); +require_once(__DIR__ . '/fixtures/test_target_course_level_shortname.php'); + +/** + * Unit tests for the manager. + * + * @package core_analytics + * @copyright 2017 David Monllaó {@link http://www.davidmonllao.com} + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class analytics_manager_testcase extends advanced_testcase { + + /** + * test_deleted_context + */ + public function test_deleted_context() { + global $DB; + + $this->resetAfterTest(true); + $this->setAdminuser(); + set_config('enabled_stores', 'logstore_standard', 'tool_log'); + + $target = \core_analytics\manager::get_target('test_target_course_level_shortname'); + $indicators = array('test_indicator_max', 'test_indicator_min', 'test_indicator_fullname'); + foreach ($indicators as $key => $indicator) { + $indicators[$key] = \core_analytics\manager::get_indicator($indicator); + } + + $model = \core_analytics\model::create($target, $indicators); + $modelobj = $model->get_model_obj(); + + $coursepredict1 = $this->getDataGenerator()->create_course(array('visible' => 0)); + $coursepredict2 = $this->getDataGenerator()->create_course(array('visible' => 0)); + $coursetrain1 = $this->getDataGenerator()->create_course(array('visible' => 1)); + $coursetrain2 = $this->getDataGenerator()->create_course(array('visible' => 1)); + + $model->enable('\core\analytics\time_splitting\no_splitting'); + + $model->train(); + $model->predict(); + + // Generate a prediction action to confirm that it is deleted when there is an important update. + $predictions = $DB->get_records('analytics_predictions'); + $prediction = reset($predictions); + $prediction = new \core_analytics\prediction($prediction, array('whatever' => 'not used')); + $prediction->action_executed(\core_analytics\prediction::ACTION_FIXED, $model->get_target()); + + $predictioncontextid = $prediction->get_prediction_data()->contextid; + + $npredictions = $DB->count_records('analytics_predictions', array('contextid' => $predictioncontextid)); + $npredictionactions = $DB->count_records('analytics_prediction_actions', + array('predictionid' => $prediction->get_prediction_data()->id)); + $nindicatorcalc = $DB->count_records('analytics_indicator_calc', array('contextid' => $predictioncontextid)); + + \core_analytics\manager::cleanup(); + + // Nothing is incorrectly deleted. + $this->assertEquals($npredictions, $DB->count_records('analytics_predictions', + array('contextid' => $predictioncontextid))); + $this->assertEquals($npredictionactions, $DB->count_records('analytics_prediction_actions', + array('predictionid' => $prediction->get_prediction_data()->id))); + $this->assertEquals($nindicatorcalc, $DB->count_records('analytics_indicator_calc', + array('contextid' => $predictioncontextid))); + + // Now we delete a context, the course predictions and prediction actions should be deleted. + $deletedcontext = \context::instance_by_id($predictioncontextid); + delete_course($deletedcontext->instanceid, false); + + \core_analytics\manager::cleanup(); + + $this->assertEmpty($DB->count_records('analytics_predictions', array('contextid' => $predictioncontextid))); + $this->assertEmpty($DB->count_records('analytics_prediction_actions', + array('predictionid' => $prediction->get_prediction_data()->id))); + $this->assertEmpty($DB->count_records('analytics_indicator_calc', array('contextid' => $predictioncontextid))); + + set_config('enabled_stores', '', 'tool_log'); + get_log_manager(true); + } + + /** + * test_deleted_analysable + */ + public function test_deleted_analysable() { + global $DB; + + $this->resetAfterTest(true); + $this->setAdminuser(); + set_config('enabled_stores', 'logstore_standard', 'tool_log'); + + $target = \core_analytics\manager::get_target('test_target_course_level_shortname'); + $indicators = array('test_indicator_max', 'test_indicator_min', 'test_indicator_fullname'); + foreach ($indicators as $key => $indicator) { + $indicators[$key] = \core_analytics\manager::get_indicator($indicator); + } + + $model = \core_analytics\model::create($target, $indicators); + $modelobj = $model->get_model_obj(); + + $coursepredict1 = $this->getDataGenerator()->create_course(array('visible' => 0)); + $coursepredict2 = $this->getDataGenerator()->create_course(array('visible' => 0)); + $coursetrain1 = $this->getDataGenerator()->create_course(array('visible' => 1)); + $coursetrain2 = $this->getDataGenerator()->create_course(array('visible' => 1)); + + $model->enable('\core\analytics\time_splitting\no_splitting'); + + $model->train(); + $model->predict(); + + $npredictsamples = $DB->count_records('analytics_predict_samples'); + $ntrainsamples = $DB->count_records('analytics_train_samples'); + + // Now we delete an analysable, stored predict and training samples should be deleted. + $deletedcontext = \context_course::instance($coursepredict1->id); + delete_course($coursepredict1, false); + + \core_analytics\manager::cleanup(); + + $this->assertEmpty($DB->count_records('analytics_predict_samples', array('analysableid' => $coursepredict1->id))); + $this->assertEmpty($DB->count_records('analytics_train_samples', array('analysableid' => $coursepredict1->id))); + + set_config('enabled_stores', '', 'tool_log'); + get_log_manager(true); + } + +} diff --git a/lang/en/admin.php b/lang/en/admin.php index c342d025fe3..b294490d7e1 100644 --- a/lang/en/admin.php +++ b/lang/en/admin.php @@ -1086,6 +1086,7 @@ $string['tablesnosave'] = 'Changes in tables above are saved automatically.'; $string['tabselectedtofront'] = 'On tables with tabs, should the row with the currently selected tab be placed at the front'; $string['tabselectedtofronttext'] = 'Bring selected tab row to front'; $string['testsiteupgradewarning'] = 'You are currently using the {$a} test site, to upgrade it properly use the command line interface tool'; +$string['taskanalyticscleanup'] = 'Analytics cleanup'; $string['taskautomatedbackup'] = 'Automated backups'; $string['taskbackupcleanup'] = 'Clean backup tables and logs'; $string['taskbadgescron'] = 'Award badges'; diff --git a/lib/classes/task/analytics_cleanup_task.php b/lib/classes/task/analytics_cleanup_task.php new file mode 100644 index 00000000000..0824d93e299 --- /dev/null +++ b/lib/classes/task/analytics_cleanup_task.php @@ -0,0 +1,55 @@ +. + +/** + * A scheduled task. + * + * @package core + * @copyright 2017 David Monllao {@link http://www.davidmonllao.com} + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +namespace core\task; + +defined('MOODLE_INTERNAL') || die(); + +/** + * Delete stale records from analytics tables. + * + * @package core + * @copyright 2017 David Monllao {@link http://www.davidmonllao.com} + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class analytics_cleanup_task extends \core\task\scheduled_task { + + /** + * Get a descriptive name for this task (shown to admins). + * + * @return string + */ + public function get_name() { + return get_string('taskanalyticscleanup', 'admin'); + } + + /** + * Executes the clean up task. + * + * @return void + */ + public function execute() { + $models = \core_analytics\manager::cleanup(); + } +} diff --git a/lib/db/tasks.php b/lib/db/tasks.php index bdb87087fd5..b550c54dda0 100644 --- a/lib/db/tasks.php +++ b/lib/db/tasks.php @@ -356,4 +356,13 @@ $tasks = array( 'dayofweek' => '*', 'month' => '*' ), + array( + 'classname' => 'core\task\analytics_cleanup_task', + 'blocking' => 0, + 'minute' => 'R', + 'hour' => '*', + 'day' => '*', + 'dayofweek' => '*', + 'month' => '*' + ), ); From abafbc840555931b2ee1a392d6ccf466f87c7cfa Mon Sep 17 00:00:00 2001 From: David Monllao Date: Wed, 6 Sep 2017 18:42:46 +0200 Subject: [PATCH 4/4] MDL-59630 analytics: mlbackend model directories deletion --- analytics/classes/model.php | 54 ++++++++++++++++------ analytics/classes/predictor.php | 33 +++++++++++++ analytics/tests/model_test.php | 16 +++++-- lib/mlbackend/php/classes/processor.php | 21 +++++++++ lib/mlbackend/python/classes/processor.php | 21 +++++++++ 5 files changed, 127 insertions(+), 18 deletions(-) diff --git a/analytics/classes/model.php b/analytics/classes/model.php index d8b1ab2216f..4bf9ea1b9db 100644 --- a/analytics/classes/model.php +++ b/analytics/classes/model.php @@ -435,14 +435,20 @@ class model { if ($this->model->timesplitting !== $timesplittingid || $this->model->indicators !== $indicatorsstr) { + + // Delete generated predictions before changing the model version. + $this->clear_model(); + + // It needs to be reset as the version changes. + $this->uniqueid = null; + // We update the version of the model so different time splittings are not mixed up. $this->model->version = $now; - // Delete generated predictions. - $this->clear_model(); - // Reset trained flag. - $this->model->trained = 0; + if (!$this->is_static()) { + $this->model->trained = 0; + } } else if ($this->model->enabled != $enabled) { // We purge the cached contexts with insights as some will not be visible anymore. @@ -456,9 +462,6 @@ class model { $this->model->usermodified = $USER->id; $DB->update_record('analytics_models', $this->model); - - // It needs to be reset (just in case, we may already used it). - $this->uniqueid = null; } /** @@ -472,6 +475,11 @@ class model { \core_analytics\manager::check_can_manage_models(); $this->clear_model(); + + // Method self::clear_model is already clearing the current model version. + $predictor = \core_analytics\manager::get_predictions_processor(); + $predictor->delete_output_dir($this->get_output_dir(array(), true)); + $DB->delete_records('analytics_models', array('id' => $this->model->id)); $DB->delete_records('analytics_models_log', array('modelid' => $this->model->id)); } @@ -974,13 +982,23 @@ class model { throw new \moodle_exception('errorinvalidtimesplitting', 'analytics'); } + // Delete generated predictions before changing the model version. + $this->clear_model(); + + // It needs to be reset as the version changes. + $this->uniqueid = null; + $this->model->timesplitting = $timesplittingid; $this->model->version = $now; + + // Reset trained flag. + if (!$this->is_static()) { + $this->model->trained = 0; + } } // Purge pages with insights as this may change things. - if ($timesplittingid && $timesplittingid !== $this->model->timesplitting || - $this->model->enabled != 1) { + if ($this->model->enabled != 1) { $this->purge_insights_cache(); } @@ -989,9 +1007,6 @@ class model { // We don't always update timemodified intentionally as we reserve it for target, indicators or timesplitting updates. $DB->update_record('analytics_models', $this->model); - - // It needs to be reset (just in case, we may already used it). - $this->uniqueid = null; } /** @@ -1229,9 +1244,10 @@ class model { * models/$model->id/$model->version/execution * * @param array $subdirs + * @param bool $onlymodelid Preference over $subdirs * @return string */ - protected function get_output_dir($subdirs = array()) { + protected function get_output_dir($subdirs = array(), $onlymodelid = false) { global $CFG; $subdirstr = ''; @@ -1245,8 +1261,12 @@ class model { $outputdir = rtrim($CFG->dataroot, '/') . DIRECTORY_SEPARATOR . 'models'; } - // Append model id and version + subdirs. - $outputdir .= DIRECTORY_SEPARATOR . $this->model->id . DIRECTORY_SEPARATOR . $this->model->version . $subdirstr; + // Append model id + $outputdir .= DIRECTORY_SEPARATOR . $this->model->id; + if (!$onlymodelid) { + // Append version + subdirs. + $outputdir .= DIRECTORY_SEPARATOR . $this->model->version . $subdirstr; + } make_writable_directory($outputdir); @@ -1411,6 +1431,10 @@ class model { private function clear_model() { global $DB; + // Delete current model version stored stuff. + $predictor = \core_analytics\manager::get_predictions_processor(); + $predictor->clear_model($this->get_unique_id(), $this->get_output_dir()); + $predictionids = $DB->get_fieldset_select('analytics_predictions', 'id', 'modelid = :modelid', array('modelid' => $this->get_id())); if ($predictionids) { diff --git a/analytics/classes/predictor.php b/analytics/classes/predictor.php index 4b4548b4084..4dcb4913b33 100644 --- a/analytics/classes/predictor.php +++ b/analytics/classes/predictor.php @@ -41,4 +41,37 @@ interface predictor { * @return bool */ public function is_ready(); + + /** + * Delete all stored information of the current model id. + * + * This method is called when there are important changes to a model, + * all previous training algorithms using that version of the model + * should be deleted. + * + * In case you want to perform extra security measures before deleting + * a directory you can check that $modelversionoutputdir subdirectories + * can only be named 'execution', 'evaluation' or 'testing'. + * + * @param string $uniqueid The site model unique id string + * @param string $modelversionoutputdir The output dir of this model version + * @return null + */ + public function clear_model($uniqueid, $modelversionoutputdir); + + /** + * Delete the output directory. + * + * This method is called when a model is completely deleted. + * + * In case you want to perform extra security measures before deleting + * a directory you can check that the subdirectories are timestamps + * (the model version) and each of this subdirectories' subdirectories + * can only be named 'execution', 'evaluation' or 'testing'. + * + * @param string $modeloutputdir The model directory id (parent of all model versions subdirectories). + * @return null + */ + public function delete_output_dir($modeloutputdir); + } diff --git a/analytics/tests/model_test.php b/analytics/tests/model_test.php index 48bd6300762..fcb7eaceb83 100644 --- a/analytics/tests/model_test.php +++ b/analytics/tests/model_test.php @@ -99,6 +99,9 @@ class analytics_model_testcase extends advanced_testcase { // Fake evaluation results record to check that it is actually deleted. $this->add_fake_log(); + $modeloutputdir = $this->model->get_output_dir(array(), true); + $this->assertTrue(is_dir($modeloutputdir)); + // Generate a prediction action to confirm that it is deleted when there is an important update. $predictions = $DB->get_records('analytics_predictions'); $prediction = reset($predictions); @@ -113,6 +116,7 @@ class analytics_model_testcase extends advanced_testcase { $this->assertEmpty($DB->count_records('analytics_train_samples')); $this->assertEmpty($DB->count_records('analytics_predict_samples')); $this->assertEmpty($DB->count_records('analytics_used_files')); + $this->assertFalse(is_dir($modeloutputdir)); set_config('enabled_stores', '', 'tool_log'); get_log_manager(true); @@ -146,8 +150,13 @@ class analytics_model_testcase extends advanced_testcase { $prediction = new \core_analytics\prediction($prediction, array('whatever' => 'not used')); $prediction->action_executed(\core_analytics\prediction::ACTION_FIXED, $this->model->get_target()); + $modelversionoutputdir = $this->model->get_output_dir(); + $this->assertTrue(is_dir($modelversionoutputdir)); + // Update to an empty time splitting method to force clear_model execution. $this->model->update(1, false, ''); + $this->assertFalse(is_dir($modelversionoutputdir)); + // Restore previous time splitting method. $this->model->enable('\core\analytics\time_splitting\no_splitting'); @@ -186,7 +195,7 @@ class analytics_model_testcase extends advanced_testcase { $modeldir = $dir . DIRECTORY_SEPARATOR . $this->modelobj->id . DIRECTORY_SEPARATOR . $this->modelobj->version; $this->assertEquals($modeldir, $this->model->get_output_dir()); - $this->assertEquals($modeldir . DIRECTORY_SEPARATOR . 'asd', $this->model->get_output_dir(array('asd'))); + $this->assertEquals($modeldir . DIRECTORY_SEPARATOR . 'testing', $this->model->get_output_dir(array('testing'))); } public function test_unique_id() { @@ -280,10 +289,11 @@ class testable_model extends \core_analytics\model { * get_output_dir * * @param array $subdirs + * @param bool $onlymodelid * @return string */ - public function get_output_dir($subdirs = array()) { - return parent::get_output_dir($subdirs); + public function get_output_dir($subdirs = array(), $onlymodelid = false) { + return parent::get_output_dir($subdirs, $onlymodelid); } /** diff --git a/lib/mlbackend/php/classes/processor.php b/lib/mlbackend/php/classes/processor.php index 9cbbdf3584d..9a84c5cb88f 100644 --- a/lib/mlbackend/php/classes/processor.php +++ b/lib/mlbackend/php/classes/processor.php @@ -72,6 +72,27 @@ class processor implements \core_analytics\classifier, \core_analytics\regressor return true; } + /** + * Delete the stored models. + * + * @param string $uniqueid + * @param string $modelversionoutputdir + * @return null + */ + public function clear_model($uniqueid, $modelversionoutputdir) { + remove_dir($modelversionoutputdir); + } + + /** + * Delete the output directory. + * + * @param string $modeloutputdir + * @return null + */ + public function delete_output_dir($modeloutputdir) { + remove_dir($modeloutputdir); + } + /** * Train this processor classification model using the provided supervised learning dataset. * diff --git a/lib/mlbackend/python/classes/processor.php b/lib/mlbackend/python/classes/processor.php index faa9f6cf61b..1ba59c8ff5a 100644 --- a/lib/mlbackend/python/classes/processor.php +++ b/lib/mlbackend/python/classes/processor.php @@ -94,6 +94,27 @@ class processor implements \core_analytics\classifier, \core_analytics\regresso return get_string('pythonpackagenotinstalled', 'mlbackend_python', $cmd); } + /** + * Delete the model version output directory. + * + * @param string $uniqueid + * @param string $modelversionoutputdir + * @return null + */ + public function clear_model($uniqueid, $modelversionoutputdir) { + remove_dir($modelversionoutputdir); + } + + /** + * Delete the model output directory. + * + * @param string $modeloutputdir + * @return null + */ + public function delete_output_dir($modeloutputdir) { + remove_dir($modeloutputdir); + } + /** * Trains a machine learning algorithm with the provided dataset. *