-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Keep same extension when created tmp config file #885
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,6 +79,40 @@ def test_trial_working_dir_is_created(config): | |
shutil.rmtree(trial.working_dir) | ||
|
||
|
||
@pytest.mark.usefixtures("storage") | ||
@pytest.mark.parametrize( | ||
"config_path_name", | ||
["yaml_sample_path", "json_sample_path", "unknown_type_template_path"], | ||
) | ||
def test_trial_config_file_is_created_with_correct_ext( | ||
config, config_path_name, request | ||
): | ||
"""Check that trial config file is created with correct extension.""" | ||
config_path = request.getfixturevalue(config_path_name) | ||
config["metadata"]["user_args"].insert(1, f"--config={config_path}") | ||
backward.populate_space(config) | ||
exp = experiment_builder.build(**config) | ||
|
||
trial = exp.space.sample()[0] | ||
|
||
exp.register_trial(trial, status="reserved") | ||
|
||
assert not os.path.exists(trial.working_dir) | ||
|
||
con = Consumer(exp) | ||
config_file, results_file = con._prepare_config(trial, trial.working_dir) | ||
|
||
assert os.path.exists(trial.working_dir) | ||
assert os.path.exists(config_file.name) | ||
|
||
_, original_ext = os.path.splitext(config_path) | ||
_, tmp_ext = os.path.splitext(config_file.name) | ||
|
||
assert original_ext == tmp_ext | ||
|
||
shutil.rmtree(trial.working_dir) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is it necessary to do this? Shouldn't the working directory in the config always be based on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's because of this line: https://github.com/Epistimio/orion/pull/885/files#diff-5fe3b7fe336f5a8a566e677336db8fc5f708b6da4e5662fef48c7bfc95fc1c42R34. We could perhaps convert it to |
||
|
||
|
||
def setup_code_change_mock(config, monkeypatch, ignore_code_changes): | ||
"""Mock create experiment and trials, and infer_versioning_metadata""" | ||
exp = experiment_builder.build(**config) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I know your stance is to add type hints to new modules, but I think new methods might also be a good opportunity to add type hints.