From b9eb1894e6edc52d9a54acecae1a6cd68d4e009c Mon Sep 17 00:00:00 2001 From: dbeltran Date: Thu, 7 Mar 2024 15:40:48 +0100 Subject: [PATCH 1/3] Changes "backup error message" --- autosubmit/autosubmit.py | 4 +-- autosubmit/job/job_list.py | 44 ++++++++++++++------------ autosubmit/job/job_list_persistence.py | 9 ++++-- 3 files changed, 31 insertions(+), 26 deletions(-) diff --git a/autosubmit/autosubmit.py b/autosubmit/autosubmit.py index bb243e0d5..1bcf000cb 100644 --- a/autosubmit/autosubmit.py +++ b/autosubmit/autosubmit.py @@ -2476,12 +2476,10 @@ class Autosubmit: except AutosubmitCritical as e: raise except BaseException as e: - raise AutosubmitCritical("Error while checking the configuration files or loading the job_list", 7040, - str(e)) + raise finally: if profile: profiler.stop() - try: jobs = [] if not isinstance(job_list, type([])): diff --git a/autosubmit/job/job_list.py b/autosubmit/job/job_list.py index 9f5fbfcc4..cbd7e21af 100644 --- a/autosubmit/job/job_list.py +++ b/autosubmit/job/job_list.py @@ -204,7 +204,7 @@ class JobList(object): chunk_list = list(range(chunk_ini, num_chunks + 1)) self._chunk_list = chunk_list try: - self.graph = self.load() + self.graph = self.load(create) if type(self.graph) is not DiGraph: self.graph = nx.DiGraph() except AutosubmitCritical: @@ -213,8 +213,6 @@ class JobList(object): self.graph = nx.DiGraph() self._dic_jobs = DicJobs(date_list, member_list, chunk_list, date_format, default_retrials, as_conf) self._dic_jobs.graph = self.graph - if show_log: - Log.info("Creating jobs...") if len(self.graph.nodes) > 0: if show_log: Log.info("Load finished") @@ -246,6 +244,8 @@ class JobList(object): os.remove(os.path.join(self._persistence_path, self._persistence_file + "_backup.pkl")) new = True # This generates the job object and also finds if dic_jobs has modified from previous iteration in order to expand the workflow + if show_log: + Log.info("Creating jobs...") self._create_jobs(self._dic_jobs, 0, default_job_type) # not needed anymore all data is inside their correspondent sections in dic_jobs # This dic_job is key to the dependencies management as they're ordered by date[member[chunk]] @@ -2312,32 +2312,36 @@ class JobList(object): "Autosubmit will use a backup for recover the job_list", 6010) return list() - def load(self): + def load(self, create=False, backup=""): """ Recreates a stored job list from the persistence :return: loaded job list object :rtype: JobList """ - Log.info("Loading JobList") + if backup == "": + Log.info("Loading JobList") try: - return self._persistence.load(self._persistence_path, self._persistence_file) + return self._persistence.load(self._persistence_path, self._persistence_file + backup) except AutosubmitCritical: raise - except: - Log.printlog( - "Autosubmit will use a backup for recover the job_list", 6010) - return self.backup_load() - - def backup_load(self): - """ - Recreates a stored job list from the persistence - - :return: loaded job list object - :rtype: JobList - """ - Log.info("Loading backup JobList") - return self._persistence.load(self._persistence_path, self._persistence_file + "_backup") + except ValueError as e: + if not create: + raise AutosubmitCritical(f'JobList could not be loaded due pkl being saved with a different version of Autosubmit or Python version. {e}') + else: + Log.warning(f'Job list will be created from scratch due pkl being saved with a different version of Autosubmit or Python version. {e}') + except BaseException as e: + if backup == "": + Log.printlog( + "Autosubmit will use a backup for recover the job_list", 6010) + return self.load(create, "_backup") + else: + if not create: + raise AutosubmitCritical( + f'JobList backup could not be loaded due: {e}') + else: + Log.warning( + f'Joblist backup will be created from scratch due: {e}') def save(self): """ diff --git a/autosubmit/job/job_list_persistence.py b/autosubmit/job/job_list_persistence.py index 948f21c01..1a271a1fc 100644 --- a/autosubmit/job/job_list_persistence.py +++ b/autosubmit/job/job_list_persistence.py @@ -78,9 +78,12 @@ class JobListPersistencePkl(JobListPersistence): # copy the path to a tmp file randomseed to avoid corruption path_tmp = f'{path}.tmp_{os.urandom(8).hex()}' shutil.copy(path, path_tmp) - with open(path_tmp, 'rb') as fd: - graph = pickle.load(fd) - os.remove(path_tmp) + try: + with open(path_tmp, 'rb') as fd: + graph = pickle.load(fd) + except: + os.remove(path_tmp) + raise for u in ( node for node in graph ): # Set after the dependencies are set graph.nodes[u]["job"].children = set() -- GitLab From e38bedf500e10fc33f90fce778da1f454b1e4cbf Mon Sep 17 00:00:00 2001 From: dbeltran Date: Thu, 7 Mar 2024 16:31:50 +0100 Subject: [PATCH 2/3] Changed the location of pkl.tmp as pkl is not Writeable for the users --- autosubmit/job/job_list.py | 2 +- autosubmit/job/job_list_persistence.py | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/autosubmit/job/job_list.py b/autosubmit/job/job_list.py index cbd7e21af..ec71b0ab5 100644 --- a/autosubmit/job/job_list.py +++ b/autosubmit/job/job_list.py @@ -2341,7 +2341,7 @@ class JobList(object): f'JobList backup could not be loaded due: {e}') else: Log.warning( - f'Joblist backup will be created from scratch due: {e}') + f'Joblist will be created from scratch due: {e}') def save(self): """ diff --git a/autosubmit/job/job_list_persistence.py b/autosubmit/job/job_list_persistence.py index 1a271a1fc..e232102c0 100644 --- a/autosubmit/job/job_list_persistence.py +++ b/autosubmit/job/job_list_persistence.py @@ -67,6 +67,8 @@ class JobListPersistencePkl(JobListPersistence): """ path = os.path.join(persistence_path, persistence_file + '.pkl') + path_tmp = os.path.join(persistence_path[:-3]+"tmp", persistence_file + f'.pkl.tmp_{os.urandom(8).hex()}') + try: open(path).close() except PermissionError: @@ -76,11 +78,11 @@ class JobListPersistencePkl(JobListPersistence): return list() else: # copy the path to a tmp file randomseed to avoid corruption - path_tmp = f'{path}.tmp_{os.urandom(8).hex()}' - shutil.copy(path, path_tmp) try: + shutil.copy(path, path_tmp) with open(path_tmp, 'rb') as fd: graph = pickle.load(fd) + os.remove(path_tmp) except: os.remove(path_tmp) raise -- GitLab From 26691ec4710cea28122c88284c1dda10f2e5e5cc Mon Sep 17 00:00:00 2001 From: dbeltran Date: Fri, 8 Mar 2024 11:33:19 +0100 Subject: [PATCH 3/3] added unit test changed code based on feedback --- autosubmit/job/job_list.py | 30 ++++++------ autosubmit/job/job_list_persistence.py | 11 ++--- test/unit/test_job_list.py | 68 ++++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 20 deletions(-) diff --git a/autosubmit/job/job_list.py b/autosubmit/job/job_list.py index ec71b0ab5..2bf7933f6 100644 --- a/autosubmit/job/job_list.py +++ b/autosubmit/job/job_list.py @@ -2312,36 +2312,38 @@ class JobList(object): "Autosubmit will use a backup for recover the job_list", 6010) return list() - def load(self, create=False, backup=""): + def load(self, create=False, backup=False): """ Recreates a stored job list from the persistence :return: loaded job list object :rtype: JobList """ - if backup == "": - Log.info("Loading JobList") try: - return self._persistence.load(self._persistence_path, self._persistence_file + backup) - except AutosubmitCritical: - raise + if not backup: + Log.info("Loading JobList") + return self._persistence.load(self._persistence_path, self._persistence_file) + else: + return self._persistence.load(self._persistence_path, self._persistence_file + "_backup") except ValueError as e: if not create: raise AutosubmitCritical(f'JobList could not be loaded due pkl being saved with a different version of Autosubmit or Python version. {e}') else: Log.warning(f'Job list will be created from scratch due pkl being saved with a different version of Autosubmit or Python version. {e}') + except PermissionError as e: + if not create: + raise AutosubmitCritical(f'JobList could not be loaded due to permission error. {e}') + else: + Log.warning(f'Job list will be created from scratch due to permission error. {e}') except BaseException as e: - if backup == "": - Log.printlog( - "Autosubmit will use a backup for recover the job_list", 6010) - return self.load(create, "_backup") + if not backup: + Log.debug("Autosubmit will use a backup to recover the job_list") + return self.load(create, True) else: if not create: - raise AutosubmitCritical( - f'JobList backup could not be loaded due: {e}') + raise AutosubmitCritical(f"JobList could not be loaded due: {e}\nAutosubmit won't do anything") else: - Log.warning( - f'Joblist will be created from scratch due: {e}') + Log.warning(f'Joblist will be created from scratch due: {e}') def save(self): """ diff --git a/autosubmit/job/job_list_persistence.py b/autosubmit/job/job_list_persistence.py index e232102c0..791de7864 100644 --- a/autosubmit/job/job_list_persistence.py +++ b/autosubmit/job/job_list_persistence.py @@ -72,20 +72,19 @@ class JobListPersistencePkl(JobListPersistence): try: open(path).close() except PermissionError: - raise AutosubmitCritical(f'Permission denied to read {path}', 7012) + Log.warning(f'Permission denied to read {path}') + raise except FileNotFoundError: - Log.printlog(f'File {path} does not exist. ',Log.WARNING) - return list() + Log.warning(f'File {path} does not exist. ') + raise else: # copy the path to a tmp file randomseed to avoid corruption try: shutil.copy(path, path_tmp) with open(path_tmp, 'rb') as fd: graph = pickle.load(fd) + finally: os.remove(path_tmp) - except: - os.remove(path_tmp) - raise for u in ( node for node in graph ): # Set after the dependencies are set graph.nodes[u]["job"].children = set() diff --git a/test/unit/test_job_list.py b/test/unit/test_job_list.py index d02322503..0dc87554c 100644 --- a/test/unit/test_job_list.py +++ b/test/unit/test_job_list.py @@ -1,3 +1,4 @@ +import os from unittest import TestCase from copy import copy import networkx @@ -15,6 +16,7 @@ from autosubmit.job.job_common import Type from autosubmit.job.job_list import JobList from autosubmit.job.job_list_persistence import JobListPersistencePkl from autosubmitconfigparser.config.yamlparser import YAMLParserFactory +from log.log import AutosubmitCritical class TestJobList(TestCase): @@ -66,6 +68,72 @@ class TestJobList(TestCase): def tearDown(self) -> None: shutil.rmtree(self.temp_directory) + def test_load(self): + as_conf = Mock() + as_conf.experiment_data = dict() + parser_mock = Mock() + parser_mock.read = Mock() + factory = YAMLParserFactory() + factory.create_parser = Mock(return_value=parser_mock) + date_list = ['fake-date1', 'fake-date2'] + member_list = ['fake-member1', 'fake-member2'] + num_chunks = 999 + parameters = {'fake-key': 'fake-value', + 'fake-key2': 'fake-value2'} + with tempfile.TemporaryDirectory() as temp_dir: + job_list = self.new_job_list(factory, temp_dir) + FakeBasicConfig.LOCAL_ROOT_DIR = str(temp_dir) + Path(temp_dir, self.experiment_id).mkdir() + for path in [f'{self.experiment_id}/tmp', f'{self.experiment_id}/tmp/ASLOGS', + f'{self.experiment_id}/tmp/ASLOGS_{self.experiment_id}', f'{self.experiment_id}/proj', + f'{self.experiment_id}/conf', f'{self.experiment_id}/pkl']: + Path(temp_dir, path).mkdir() + job_list.changes = Mock(return_value=['random_section', 'random_section']) + as_conf.detailed_deep_diff = Mock(return_value={}) + # as_conf.get_member_list = Mock(return_value=member_list) + # act + job_list.generate( + as_conf=as_conf, + date_list=date_list, + member_list=member_list, + num_chunks=num_chunks, + chunk_ini=1, + parameters=parameters, + date_format='H', + default_retrials=9999, + default_job_type=Type.BASH, + wrapper_jobs={}, + new=True, + create=True, + ) + job_list.save() + # Test load + job_list_to_load = self.new_job_list(factory, temp_dir) + # chmod + job_list_to_load.load(False) + self.assertEqual(job_list_to_load._job_list, job_list._job_list) + job_list_to_load.load(True) + self.assertEqual(job_list_to_load._job_list, job_list._job_list) + os.chmod(f'{temp_dir}/{self.experiment_id}/pkl/job_list_random-id.pkl', 0o000) + with self.assertRaises(AutosubmitCritical): + job_list_to_load.load(False) + job_list_to_load.load(True) + self.assertEqual(job_list_to_load._job_list, job_list._job_list) + os.chmod(f'{temp_dir}/{self.experiment_id}/pkl/job_list_random-id.pkl', 0o777) + shutil.copy(f'{temp_dir}/{self.experiment_id}/pkl/job_list_random-id.pkl',f'{temp_dir}/{self.experiment_id}/pkl/job_list_random-id_backup.pkl') + os.remove(f'{temp_dir}/{self.experiment_id}/pkl/job_list_random-id.pkl') + job_list_to_load.load(False) + self.assertEqual(job_list_to_load._job_list, job_list._job_list) + job_list_to_load.load(True) + self.assertEqual(job_list_to_load._job_list, job_list._job_list) + + + + + + + + def test_get_job_list_returns_the_right_list(self): job_list = self.job_list.get_job_list() self.assertEqual(self.job_list._job_list, job_list) -- GitLab