Repository: hu-macsy/simexpal

Issue:
Performance of no-op simex runs
I noticed that just running `simex e` on an empty file takes 300ms on my (decently fast) laptop. This seems to be mainly due to the import of external packages: base.py imports instances.py and that imports `requests`, which takes quite a bit of time. Additionally, importing `jsonschema` ist not for free.

* Only importing instances.py and `requests` when it we need them should be easy.
* For jsonschema, maybe we can cache validation results (however, that would also require us to cache validation error, or, alternatively, disable the caching when there are validation errors).

Discussion:
duylethanh: Sorry, for all the force pushing. This should be the last for now. I forgot to mention that I refactored ``base.config_for_dir()``, which lead to the removal of ``base.read_and_validate_setup()``. As I'd like to acces the ``basedir`` in ``util.validate_setup_file()`` I changed the parameters it receives, explicitly stating ``basedir``. This in turn, made ``base.read_and_validate_setup()`` unnecessary, as it only joined the ``basedir`` with the ``setup_file``.

PR ready to be reviewed.
duylethanh: I made the requested changes. I also had to make a few adjustments to the validation unit test:

- remove the assertion for ``instdir`` in the returned ``dict``, as this was moved to the ``Config`` constructor
- delete ``validation.cache`` files before running the validation, as there could already be a cache file from previous (other) tests

PR ready to be reviewed.
avdgrinten: Can you clarify why we remove the `validation.cache` file in tests? To ensure that the test re-validate even if the file was validated correctly? Or is there any correctness issue without removing the file?

Patch:
diff --git a/simexpal/base.py b/simexpal/base.py
index 44b9519..65457f3 100644
--- a/simexpal/base.py
+++ b/simexpal/base.py
@@ -9,7 +9,6 @@ import sys
 import tempfile
 import warnings
 
-from . import instances
 from . import util
 
 DEFAULT_DEV_BUILD_NAME = '_dev'
@@ -120,6 +119,9 @@ class Config:
 
 						yield Variant(self, axis_yml['axis'], variant_yml)
 
+		if 'instdir' not in self.yml:
+			self.yml['instdir'] = './instances'
+
 		for inst in sorted(construct_instances(), key=lambda inst: inst.shortname):
 			if inst.shortname in self._insts:
 				raise RuntimeError("The instance name '{}' is ambiguous".format(inst.shortname))
@@ -157,6 +159,11 @@ class Config:
 
 				if exp_yml['name'] in self._exp_infos:
 					raise RuntimeError("The experiment name '{}' is ambiguous".format(exp_yml['name']))
+
+				if exp_yml.get('output', None) == 'stdout':
+					msg = "Specifying the stdout path via 'output: stdout' is deprecated and will be removed in future " \
+							"versions. Use 'stdout: out' instead."
+					warnings.warn(msg, DeprecationWarning)
 				self._exp_infos[exp_yml['name']] = ExperimentInfo(self, exp_yml)
 
 		try:
@@ -684,6 +691,8 @@ class Instance:
 		return True
 
 	def install(self):
+		from . import instances
+
 		global DID_WARN_KONECT
 
 		if self.check_available() or self.is_fileless:
@@ -745,6 +754,9 @@ class Instance:
 
 	def run_transform(self, transform, out_path):
 		assert transform == 'to_edgelist'
+
+		from . import instances
+
 		instances.convert_to_edgelist(self._inst_yml,
 				self.fullpath, out_path + '.transf1')
 		stage = 1
@@ -1334,12 +1346,9 @@ class Run:
 			raise RuntimeError("The experiment '{}' with instance '{}' has not been started yet".format(
 				self.experiment.display_name, self.instance.shortname))
 
-def read_and_validate_setup(basedir='.', setup_file='experiments.yml'):
-	return util.validate_setup_file(os.path.join(basedir, setup_file))
-
 def config_for_dir(basedir=None):
 	if basedir is None:
 		basedir = '.'
-	yml = read_and_validate_setup(basedir=basedir)
+	yml = util.validate_setup_file(basedir, 'experiments.yml', 'experiments.json')
 	return Config(os.path.abspath(basedir), yml)
 
diff --git a/simexpal/instances.py b/simexpal/instances.py
index 12b6b27..56319b6 100644
--- a/simexpal/instances.py
+++ b/simexpal/instances.py
@@ -1,7 +1,6 @@
 
 import gzip
 import os
-import requests
 import zipfile
 
 from .util import try_mkdir, expand_at_params
@@ -21,6 +20,8 @@ repos = {
 }
 
 def download_instance(inst_yml, instances_dir, filename, partial_path, ext):
+	import requests
+
 	shortname = os.path.splitext(filename)[0]
 	if 'repo' in inst_yml:
 		repo = inst_yml['repo']
diff --git a/simexpal/util.py b/simexpal/util.py
index d71056e..5af864c 100644
--- a/simexpal/util.py
+++ b/simexpal/util.py
@@ -6,8 +6,8 @@ import shutil
 import sys
 import yaml
 import json
-from jsonschema import Draft7Validator
-import warnings
+import tempfile
+
 
 def expand_at_params(s, fn, listfn=None):
 	def subfn(m):
@@ -78,15 +78,21 @@ def read_setup_file(setup_file):
 
 	with open(setup_file, 'r') as f:
 		setup_dict = yaml.load(f, Loader=YmlLoader)
-	return setup_dict
+		last_mod = os.fstat(f.fileno()).st_mtime
+	return setup_dict, last_mod
+
+def read_json_file(json_file):
+	with open(json_file, 'r') as f:
+		json_dict = json.load(f)
+		last_mod = os.fstat(f.fileno()).st_mtime
+	return json_dict, last_mod
 
-def validate_setup_file(setup_file):
+def validate_setup_file(basedir, setup_file, setup_file_schema_name):
 	""" Reads, validates and sanitizes the setup file
 	"""
 
-	def _validate_dict(dictionary, source, schema_path):
-		with open(schema_path, 'r') as f:
-			schema = json.load(f)
+	def _validate_dict(dictionary, source, schema):
+		from jsonschema import Draft7Validator
 
 		validator = Draft7Validator(schema)
 		validation_errors = list(validator.iter_errors(dictionary))
@@ -110,33 +116,77 @@ def validate_setup_file(setup_file):
 							print("\nValidation errors in subschema [{}]:".format(cur_schema_index))
 						print(sub_error.message)
 					print()
-			sys.exit(1)
+			return False
+		return True
 
 	cur_file_path = os.path.abspath(os.path.dirname(__file__))
 
-	exp_yml_dict = read_setup_file(setup_file)
-	schema_path = os.path.join(cur_file_path, "schemes", "experiments.json")
-	_validate_dict(exp_yml_dict, "experiments.yml", schema_path)
-
+	validation_cache_dict = {}
 	try:
-		launchers_yml_dict = read_setup_file(os.path.expanduser('~/.simexpal/launchers.yml'))
-		schema_path = os.path.join(cur_file_path, "schemes", "launchers.json")
-		_validate_dict(launchers_yml_dict, "launchers.yml", schema_path)
+		with open(os.path.join(basedir, 'validation.cache'), 'r') as f:
+			validation_cache_dict = json.load(f)
 	except FileNotFoundError:
 		pass
 
-	for exp in exp_yml_dict.get('experiments', []):
-		if exp.get('output', None) == 'stdout':
-			msg = "Specifying the stdout path via 'output: stdout' is deprecated and will be removed in future " \
-					"versions. Use 'stdout: out' instead."
-			warnings.warn(msg, DeprecationWarning)
-
-			break
-
-	if 'instdir' not in exp_yml_dict:
-		exp_yml_dict['instdir'] = './instances'
+	# Validate setup file and potentially cache results.
+	setup_file_path = os.path.join(basedir, setup_file)
+	setup_file_dict, setup_file_last_mod = read_setup_file(setup_file_path)
+	setup_file_schema_path = os.path.join(cur_file_path, 'schemes', setup_file_schema_name)
+	setup_file_schema, setup_file_schema_last_mod = read_json_file(setup_file_schema_path)
+
+	setup_file_is_valid = None
+	if (setup_file not in validation_cache_dict
+		or setup_file_last_mod != validation_cache_dict[setup_file]
+		or setup_file_schema_name not in validation_cache_dict
+		or setup_file_schema_last_mod != validation_cache_dict[setup_file_schema_name]):
+
+		setup_file_is_valid = _validate_dict(setup_file_dict, setup_file, setup_file_schema)
+		if setup_file_is_valid:
+			validation_cache_dict[setup_file] = setup_file_last_mod
+			validation_cache_dict[setup_file_schema_name] = setup_file_schema_last_mod
+
+	# Validate launchers.yml file and potentially cache results.
+	launchers_yml_is_valid = None
+	try:
+		launchers_yml_dict, launchers_yml_last_mod = read_setup_file(os.path.expanduser('~/.simexpal/launchers.yml'))
+		launchers_yml_schema_path = os.path.join(cur_file_path, 'schemes', 'launchers.json')
+		launchers_yml_schema, launchers_yml_schema_last_mod = read_json_file(launchers_yml_schema_path)
+
+		if ('launchers.yml' not in validation_cache_dict
+			or launchers_yml_last_mod != validation_cache_dict['launchers.yml']
+			or launchers_yml_schema_last_mod != validation_cache_dict['launchers.json']):
+
+			launchers_yml_is_valid = _validate_dict(launchers_yml_dict, 'launchers.yml', launchers_yml_schema)
+			if launchers_yml_is_valid:
+				validation_cache_dict['launchers.yml'] = launchers_yml_last_mod
+				validation_cache_dict['launchers.json'] = launchers_yml_schema_last_mod
+	except FileNotFoundError:
+		pass
 
-	return exp_yml_dict
+	writeback_cache = False
+	do_exit = False
+	if setup_file_is_valid is not None:
+		if setup_file_is_valid:
+			writeback_cache = True
+		else:
+			do_exit = True
+
+	if launchers_yml_is_valid is not None:
+		if launchers_yml_is_valid:
+			writeback_cache = True
+		else:
+			do_exit = True
+
+	if writeback_cache:
+		fd, path = tempfile.mkstemp(dir=basedir)
+		with os.fdopen(fd, 'w') as tmp:
+			json.dump(validation_cache_dict, tmp)
+		os.rename(path, 'validation.cache')
+
+	if do_exit:
+		sys.exit(1)
+
+	return setup_file_dict
 
 def compute_network_size(path, out):
 	import networkit as nk


Test patch:
diff --git a/tests/validation/test_experiments_yml.py b/tests/validation/test_experiments_yml.py
index 3f0a9f2..550a3dc 100644
--- a/tests/validation/test_experiments_yml.py
+++ b/tests/validation/test_experiments_yml.py
@@ -5,24 +5,33 @@ import pytest
 from simexpal import util
 
 file_dir = os.path.abspath(os.path.dirname(__file__))
-valid_experiments_ymls = ['/../../examples/sorting/experiments.yml',
-                          '/../../examples/sorting_cpp/experiments.yml',
-                          '/../../examples/download_instances/experiments.yml',
-                          '/experiments_ymls/valid/instances/extra_args.yml']
 
-invalid_experiments_ymls = ['/experiments_ymls/invalid/top_level-additional_property.yml',
-                            '/experiments_ymls/invalid/repo-invalid_value.yml']
+valid_experiments_ymls = ['../../examples/sorting/experiments.yml',
+                          '../../examples/sorting_cpp/experiments.yml',
+                          '../../examples/download_instances/experiments.yml',
+                          'experiments_ymls/valid/instances/extra_args.yml']
+
+invalid_experiments_ymls = ['experiments_ymls/invalid/top_level-additional_property.yml',
+                            'experiments_ymls/invalid/repo-invalid_value.yml']
+
 
 @pytest.mark.parametrize('rel_yml_path', valid_experiments_ymls)
 def test_valid_experiments_yml(rel_yml_path):
-    ret_val = util.validate_setup_file(file_dir + rel_yml_path)
+    rel_yml_dir = os.path.dirname(rel_yml_path)
+    yml_dir = os.path.join(file_dir, rel_yml_dir)
+    filename = os.path.basename(rel_yml_path)
+
+    ret_val = util.validate_setup_file(yml_dir, filename, 'experiments.json')
 
     assert isinstance(ret_val, dict)
-    assert 'instdir' in ret_val
 
 @pytest.mark.parametrize('rel_yml_path', invalid_experiments_ymls)
 def test_invalid_experiments_yml(rel_yml_path):
+    rel_yml_dir = os.path.dirname(rel_yml_path)
+    yml_dir = os.path.join(file_dir, rel_yml_dir)
+    filename = os.path.basename(rel_yml_path)
+
     with pytest.raises(SystemExit) as info:
-        util.validate_setup_file(file_dir + rel_yml_path)
+        util.validate_setup_file(yml_dir, filename, 'experiments.json')
 
     assert info.value.code == 1