From b716612642fd1148bf368af82265f2da923beb4a Mon Sep 17 00:00:00 2001 From: Xavier Bouthillier Date: Fri, 23 Jul 2021 11:22:24 -0400 Subject: [PATCH] Test consumer ignore_code_changes if evc enabled Why: If the EVC is not enabled, the consumer should always ignore the code changes. It would not make sense to raise an error between 2 trials because user's script code changed while EVC is disabled. How: If EVC is disabled, force ignore_code_changes to True in Consumer. --- src/orion/core/cli/hunt.py | 12 +- src/orion/core/evc/conflicts.py | 12 +- src/orion/core/worker/consumer.py | 13 +- .../configuration/test_all_options.py | 211 ++++++++++++------ .../core/io/test_experiment_builder.py | 29 +++ tests/unittests/core/worker/test_consumer.py | 27 ++- 6 files changed, 224 insertions(+), 80 deletions(-) diff --git a/src/orion/core/cli/hunt.py b/src/orion/core/cli/hunt.py index e05f29575e..72ad6e99e8 100644 --- a/src/orion/core/cli/hunt.py +++ b/src/orion/core/cli/hunt.py @@ -88,8 +88,10 @@ def main(args): if config.get("worker"): worker_config.update(config.get("worker")) - workon( - experiment, - ignore_code_changes=config["branching"].get("ignore_code_changes"), - **worker_config - ) + # If EVC is not enabled, we force Consumer to ignore code changes. + if not config["branching"].get("enable", orion.core.config.evc.enable): + ignore_code_changes = True + else: + ignore_code_changes = config["branching"].get("ignore_code_changes") + + workon(experiment, ignore_code_changes=ignore_code_changes, **worker_config) diff --git a/src/orion/core/evc/conflicts.py b/src/orion/core/evc/conflicts.py index 93772d9851..c6166d2ae8 100644 --- a/src/orion/core/evc/conflicts.py +++ b/src/orion/core/evc/conflicts.py @@ -1169,9 +1169,15 @@ def detect(cls, old_config, new_config, branching_config=None): old_hash_commit = old_config["metadata"].get("VCS", None) new_hash_commit = new_config["metadata"].get("VCS") - ignore_code_changes = branching_config is not None and branching_config.get( - "ignore_code_changes", False - ) + # Will be overriden by global config if not set in branching_config + ignore_code_changes = None + # Try using user defined ignore_code_changes + if branching_config is not None: + ignore_code_changes = branching_config.get("ignore_code_changes", None) + # Otherwise use global conf's ignore_code_changes + if ignore_code_changes is None: + ignore_code_changes = orion.core.config.evc.ignore_code_changes + if ignore_code_changes: log.debug("Ignoring code changes") if ( diff --git a/src/orion/core/worker/consumer.py b/src/orion/core/worker/consumer.py index 4498a46c44..208de7de27 100644 --- a/src/orion/core/worker/consumer.py +++ b/src/orion/core/worker/consumer.py @@ -95,6 +95,7 @@ def __init__( if interrupt_signal_code is None: interrupt_signal_code = orion.core.config.worker.interrupt_signal_code + # NOTE: If ignore_code_changes is None, we can assume EVC is enabled. if ignore_code_changes is None: ignore_code_changes = orion.core.config.evc.ignore_code_changes @@ -255,9 +256,6 @@ def _consume(self, trial, workdirname): return results_file def _validate_code_version(self): - if self.ignore_code_changes: - return - old_config = self.experiment.configuration new_config = copy.deepcopy(old_config) new_config["metadata"]["VCS"] = infer_versioning_metadata( @@ -268,10 +266,17 @@ def _validate_code_version(self): from orion.core.evc.conflicts import CodeConflict conflicts = list(CodeConflict.detect(old_config, new_config)) - if conflicts: + if conflicts and not self.ignore_code_changes: raise BranchingEvent( f"Code changed between execution of 2 trials:\n{conflicts[0]}" ) + elif conflicts: + log.warning( + "Code changed between execution of 2 trials. Enable EVC with option " + "`ignore_code_changes` set to False to raise an error when trials are executed " + "with different versions. For more information, see documentation at " + "https://orion.readthedocs.io/en/stable/user/config.html#experiment-version-control" + ) # pylint: disable = no-self-use def execute_process(self, cmd_args, environ): diff --git a/tests/functional/configuration/test_all_options.py b/tests/functional/configuration/test_all_options.py index 4415fd67e5..46b01dbcc1 100644 --- a/tests/functional/configuration/test_all_options.py +++ b/tests/functional/configuration/test_all_options.py @@ -139,7 +139,7 @@ def test_db_config(self, tmp_path): with self.setup_db_config(tmp_path): self.check_db_config() - @pytest.mark.usefixtures("with_user_userxyz") + @pytest.mark.usefixtures("with_user_userxyz", "version_XYZ") def test_local_config(self, tmp_path, monkeypatch): """Test that local config overrides db/global config""" update_singletons() @@ -767,7 +767,7 @@ class TestEVCConfig(ConfigurationTestSuite): "enable": False, "manual_resolution": True, "non_monitored_arguments": ["test", "local"], - "ignore_code_changes": True, + "ignore_code_changes": False, "algorithm_change": True, "code_change_type": "break", "cli_change_type": "break", @@ -780,7 +780,7 @@ class TestEVCConfig(ConfigurationTestSuite): "enable-evc": True, "manual-resolution": False, "non-monitored-arguments": "test:cmdargs", - "ignore-code-changes": False, + "ignore-code-changes": True, "algorithm-change": False, "code-change-type": "noeffect", "cli-change-type": "unsure", @@ -830,14 +830,30 @@ def check_global_config(self, tmp_path, monkeypatch): tmp_path, name, command, change_type="noeffect" ) - self._check_code_change( - monkeypatch, - name, - command, - mock_ignore_code_changes=None, - ignore_code_changes=self.config["evc"]["ignore_code_changes"], - change_type=self.config["evc"]["code_change_type"], - ) + # EVC not enabled, code change should be ignored even if option is set to True + assert self.config["evc"]["enable"] is False + with monkeypatch.context() as m: + self._check_code_change( + m, + name, + command.replace("--enable-evc ", ""), + mock_ignore_code_changes=True, + ignore_code_changes=True, + change_type=self.config["evc"]["code_change_type"], + enable_evc=False, + ) + + # EVC is enabled, option should be honored + with monkeypatch.context() as m: + self._check_code_change( + m, + name, + command, + mock_ignore_code_changes=None, + ignore_code_changes=self.config["evc"]["ignore_code_changes"], + change_type=self.config["evc"]["code_change_type"], + enable_evc=True, + ) def check_env_var_config(self, tmp_path, monkeypatch): """Check that env vars overrides global configuration""" @@ -856,7 +872,9 @@ def check_env_var_config(self, tmp_path, monkeypatch): assert not orion.core.config.evc.orion_version_change name = "env-var-test" - command = f"hunt --enable-evc --worker-max-trials 0 -n {name} python {script} -x~uniform(0,1)" + command = ( + f"hunt --worker-max-trials 0 -n {name} python {script} -x~uniform(0,1)" + ) assert orion.core.cli.main(command.split(" ")) == 0 self._check_enable(name, command, enabled=True) @@ -867,14 +885,34 @@ def check_env_var_config(self, tmp_path, monkeypatch): ) self._check_script_config_change(tmp_path, name, command, change_type="unsure") - self._check_code_change( - monkeypatch, - name, - command, - mock_ignore_code_changes=None, - ignore_code_changes=bool(self.env_vars["ORION_EVC_IGNORE_CODE_CHANGES"]), - change_type=self.env_vars["ORION_EVC_CODE_CHANGE"], - ) + # Enable EVC, ignore_code_changes is False + with monkeypatch.context() as m: + self._check_code_change( + m, + name, + command, + mock_ignore_code_changes=None, + ignore_code_changes=bool( + self.env_vars["ORION_EVC_IGNORE_CODE_CHANGES"] + ), + change_type=self.env_vars["ORION_EVC_CODE_CHANGE"], + enable_evc=True, + ) + + # Disable EVC, ignore_code_changes is True for Consumer + os.environ["ORION_EVC_ENABLE"] = "" + with monkeypatch.context() as m: + self._check_code_change( + m, + name, + command, + mock_ignore_code_changes=None, + ignore_code_changes=bool( + self.env_vars["ORION_EVC_IGNORE_CODE_CHANGES"] + ), + change_type=self.env_vars["ORION_EVC_CODE_CHANGE"], + enable_evc=False, + ) def check_db_config(self): """No Storage config in DB, no test""" @@ -911,14 +949,30 @@ def check_local_config(self, tmp_path, conf_file, monkeypatch): command, change_type=self.local["evc"]["config_change_type"], ) - self._check_code_change( - monkeypatch, - name, - command, - mock_ignore_code_changes=True, - ignore_code_changes=self.local["evc"]["ignore_code_changes"], - change_type=self.local["evc"]["code_change_type"], - ) + + # enabled evc, ignore code changes so to True + with monkeypatch.context() as m: + self._check_code_change( + m, + name, + command, + mock_ignore_code_changes=False, + ignore_code_changes=self.local["evc"]["ignore_code_changes"], + change_type=self.local["evc"]["code_change_type"], + enable_evc=True, + ) + + # disabled evc, ignore code changes so to True + with monkeypatch.context() as m: + self._check_code_change( + m, + name, + command.replace("--enable-evc ", ""), + mock_ignore_code_changes=False, + ignore_code_changes=self.local["evc"]["ignore_code_changes"], + change_type=self.local["evc"]["code_change_type"], + enable_evc=False, + ) def check_cmd_args_config(self, tmp_path, conf_file, monkeypatch): """Check that cmdargs configuration overrides global/envvars/local configuration""" @@ -927,10 +981,6 @@ def check_cmd_args_config(self, tmp_path, conf_file, monkeypatch): f"hunt --worker-max-trials 0 -c {conf_file} -n {name} " "--enable-evc " "--auto-resolution " - "--non-monitored-arguments test:cmdargs " - "--code-change-type noeffect " - "--cli-change-type unsure " - "--config-change-type break " f"python {script} -x~uniform(0,1)" ) assert orion.core.cli.main(command.split(" ")) == 0 @@ -938,18 +988,20 @@ def check_cmd_args_config(self, tmp_path, conf_file, monkeypatch): self._check_enable(name, command, enabled=True) self._check_cli_change( - name, command, change_type=self.cmdargs["cli-change-type"] + name, + command="hunt --cli-change-type unsure " + command[5:], + change_type=self.cmdargs["cli-change-type"], ) self._check_non_monitored_arguments( name, - command, + command="hunt --non-monitored-arguments test:cmdargs " + command[5:], non_monitored_arguments=self.cmdargs["non-monitored-arguments"].split(":"), ) self._check_script_config_change( tmp_path, name, - command, + command="hunt --config-change-type break " + command[5:], change_type=self.cmdargs["config-change-type"], ) @@ -964,26 +1016,42 @@ def mock_local(cmdargs): monkeypatch.setattr(orion.core.io.resolve_config, "fetch_config", mock_local) # Check that ignore_code_changes is rightly False - self._check_code_change( - monkeypatch, - name, - command, - mock_ignore_code_changes=False, - ignore_code_changes=False, - change_type=self.cmdargs["code-change-type"], - ) - - command = "hunt --ignore-code-changes " + command[5:] - - # Check that ignore_code_changes is now True - self._check_code_change( - monkeypatch, - name, - command, - mock_ignore_code_changes=True, - ignore_code_changes=True, - change_type=self.cmdargs["code-change-type"], - ) + with monkeypatch.context() as m: + self._check_code_change( + m, + name, + command="hunt --code-change-type noeffect " + command[5:], + mock_ignore_code_changes=False, + ignore_code_changes=False, + change_type=self.cmdargs["code-change-type"], + enable_evc=True, + ) + + # Check that ignore_code_changes is now True because --ignore-code-changes was added + with monkeypatch.context() as m: + self._check_code_change( + m, + name, + command="hunt --ignore-code-changes --code-change-type noeffect " + + command[5:], + mock_ignore_code_changes=True, + ignore_code_changes=True, + change_type=self.cmdargs["code-change-type"], + enable_evc=True, + ) + + # Check that ignore_code_changes is forced to True in consumer + # even if --ignore-code-changes is not passed + with monkeypatch.context() as m: + self._check_code_change( + m, + name, + command.replace("--enable-evc ", ""), + mock_ignore_code_changes=False, + ignore_code_changes=False, + change_type=self.cmdargs["code-change-type"], + enable_evc=False, + ) @with_storage_fork def _check_enable(self, name, command, enabled): @@ -1032,7 +1100,11 @@ def _check_code_change( mock_ignore_code_changes, ignore_code_changes, change_type, + enable_evc, ): + """Check if code changes are correctly ignored during experiment build and by consumer + between two trial executions. + """ # Test that code change is handled with 'no-effect' def fixed_dictionary(user_script): @@ -1058,24 +1130,35 @@ def mock_detect(old_config, new_config, branching_config=None): assert ( branching_config["ignore_code_changes"] is mock_ignore_code_changes ) - branching_config["ignore_code_changes"] = False + # branching_config["ignore_code_changes"] = False return detect(old_config, new_config, branching_config) monkeypatch.setattr( orion.core.evc.conflicts.CodeConflict, "detect", mock_detect ) + experiment = get_experiment(name) + assert orion.core.cli.main(command.split(" ")) == 0 - self._check_consumer({"ignore_code_changes": ignore_code_changes}) + self._check_consumer( + { + "ignore_code_changes": ( + (enable_evc and ignore_code_changes) or not enable_evc + ) + } + ) new_experiment = get_experiment(name) - assert new_experiment.version == experiment.version + 1 - assert new_experiment.refers["adapter"].configuration[0] == { - "of_type": "codechange", - "change_type": change_type, - } - - monkeypatch.undo() + if enable_evc and not ignore_code_changes: + assert new_experiment.version == experiment.version + 1 + assert new_experiment.refers["adapter"].configuration[0] == { + "of_type": "codechange", + "change_type": change_type, + } + elif enable_evc: # But code change ignored, so no branching event. + assert new_experiment.version == experiment.version + else: + assert new_experiment.version == experiment.version @with_storage_fork def _check_script_config_change(self, tmp_path, name, command, change_type): diff --git a/tests/unittests/core/io/test_experiment_builder.py b/tests/unittests/core/io/test_experiment_builder.py index a8fa1272cf..0bbf17922a 100644 --- a/tests/unittests/core/io/test_experiment_builder.py +++ b/tests/unittests/core/io/test_experiment_builder.py @@ -3,6 +3,7 @@ """Example usage and tests for :mod:`orion.core.io.experiment_builder`.""" import copy import datetime +import logging import pytest @@ -566,6 +567,34 @@ def test_new_experiment_w_version(self, space): assert exp.version == 1 + def test_experiment_overwritten_evc_disabled(self, parent_version_config, caplog): + """Build an existing experiment with different config, overwritting previous config.""" + parent_version_config.pop("version") + with OrionState(experiments=[parent_version_config]): + + exp = experiment_builder.load(name=parent_version_config["name"]) + assert exp.version == 1 + assert exp.configuration["algorithms"] == {"random": {"seed": None}} + + with caplog.at_level(logging.WARNING): + + exp = experiment_builder.build( + name=parent_version_config["name"], algorithms="gradient_descent" + ) + assert "Running experiment in a different state" in caplog.text + + assert exp.version == 1 + assert list(exp.configuration["algorithms"].keys())[0] == "gradient_descent" + + caplog.clear() + with caplog.at_level(logging.WARNING): + + exp = experiment_builder.load(name=parent_version_config["name"]) + assert "Running experiment in a different state" not in caplog.text + + assert exp.version == 1 + assert list(exp.configuration["algorithms"].keys())[0] == "gradient_descent" + def test_backward_compatibility_no_version(self, parent_version_config): """Branch from parent that has no version field.""" parent_version_config.pop("version") diff --git a/tests/unittests/core/worker/test_consumer.py b/tests/unittests/core/worker/test_consumer.py index 47979f3380..c240d3a0b8 100644 --- a/tests/unittests/core/worker/test_consumer.py +++ b/tests/unittests/core/worker/test_consumer.py @@ -1,6 +1,7 @@ #!/usr/bin/env python # -*- coding: utf-8 -*- """Collection of tests for :mod:`orion.core.worker.consumer`.""" +import logging import os import signal import subprocess @@ -120,16 +121,14 @@ def test_trial_working_dir_is_changed(config): assert trial.working_dir == con.working_dir + "/exp_" + trial.id -@pytest.mark.usefixtures("create_db_instance") -def test_code_changed(config, monkeypatch): - """Check that trial has its working_dir attribute changed.""" +def setup_code_change_mock(config, monkeypatch, ignore_code_changes): exp = experiment_builder.build(**config) trial = tuple_to_trial((1.0,), exp.space) exp.register_trial(trial, status="reserved") - con = Consumer(exp) + con = Consumer(exp, ignore_code_changes=ignore_code_changes) def code_changed(user_script): return dict( @@ -142,6 +141,26 @@ def code_changed(user_script): monkeypatch.setattr(consumer, "infer_versioning_metadata", code_changed) + return con, trial + + +@pytest.mark.usefixtures("create_db_instance") +def test_code_changed_evc_disabled(config, monkeypatch, caplog): + """Check that trial has its working_dir attribute changed.""" + + con, trial = setup_code_change_mock(config, monkeypatch, ignore_code_changes=True) + + with caplog.at_level(logging.WARNING): + con.consume(trial) + assert "Code changed between execution of 2 trials" in caplog.text + + +@pytest.mark.usefixtures("create_db_instance") +def test_code_changed_evc_enabled(config, monkeypatch): + """Check that trial has its working_dir attribute changed.""" + + con, trial = setup_code_change_mock(config, monkeypatch, ignore_code_changes=False) + with pytest.raises(BranchingEvent) as exc: con.consume(trial)