From caf6beb75d293d6495235f95fd6199c19a61429f Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Mon, 14 Jan 2019 17:10:00 +0100 Subject: [PATCH 1/5] Add atomic context manager To ensure that runs are not partially created --- qcodes/dataset/sqlite_base.py | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/qcodes/dataset/sqlite_base.py b/qcodes/dataset/sqlite_base.py index 9c18f02f236..223c96bdde2 100644 --- a/qcodes/dataset/sqlite_base.py +++ b/qcodes/dataset/sqlite_base.py @@ -2290,15 +2290,17 @@ def create_run(conn: ConnectionPlus, exp_id: int, name: str, - formatted_name: the name of the newly created table """ - run_counter, formatted_name, run_id = _insert_run(conn, - exp_id, - name, - guid, - parameters) - if metadata: - add_meta_data(conn, run_id, metadata) - _update_experiment_run_counter(conn, exp_id, run_counter) - _create_run_table(conn, formatted_name, parameters, values) + with atomic(conn): + run_counter, formatted_name, run_id = _insert_run(conn, + exp_id, + name, + guid, + parameters) + print(f"create run {run_id}") + if metadata: + add_meta_data(conn, run_id, metadata) + _update_experiment_run_counter(conn, exp_id, run_counter) + _create_run_table(conn, formatted_name, parameters, values) return run_counter, run_id, formatted_name From 47cf0ec86ff7161c204d5d9fd5f23a6b1307486c Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Mon, 14 Jan 2019 17:10:55 +0100 Subject: [PATCH 2/5] remove print --- qcodes/dataset/sqlite_base.py | 1 - 1 file changed, 1 deletion(-) diff --git a/qcodes/dataset/sqlite_base.py b/qcodes/dataset/sqlite_base.py index 223c96bdde2..fee22744eac 100644 --- a/qcodes/dataset/sqlite_base.py +++ b/qcodes/dataset/sqlite_base.py @@ -2296,7 +2296,6 @@ def create_run(conn: ConnectionPlus, exp_id: int, name: str, name, guid, parameters) - print(f"create run {run_id}") if metadata: add_meta_data(conn, run_id, metadata) _update_experiment_run_counter(conn, exp_id, run_counter) From 6af183c8a051ce688e56e42e13122a3ab2a29418 Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 15 Jan 2019 14:06:24 +0100 Subject: [PATCH 3/5] Add test of atomic create_run --- qcodes/tests/dataset/test_sqlite_base.py | 45 +++++++++++++++++++----- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/qcodes/tests/dataset/test_sqlite_base.py b/qcodes/tests/dataset/test_sqlite_base.py index a5e722f8fde..7da436ba258 100644 --- a/qcodes/tests/dataset/test_sqlite_base.py +++ b/qcodes/tests/dataset/test_sqlite_base.py @@ -10,6 +10,7 @@ from hypothesis import given import unicodedata import numpy as np +from unittest.mock import patch from qcodes.dataset.descriptions import RunDescriber from qcodes.dataset.dependencies import InterDependencies @@ -32,7 +33,6 @@ def test_path_to_dbfile(): - with tempfile.TemporaryDirectory() as tempdir: tempdb = os.path.join(tempdir, 'database.db') conn = mut.connect(tempdb) @@ -100,7 +100,6 @@ def test__validate_table_raises(table_name): def test_get_dependents(experiment): - x = ParamSpec('x', 'numeric') t = ParamSpec('t', 'numeric') y = ParamSpec('y', 'numeric', depends_on=['x', 't']) @@ -142,7 +141,8 @@ def test_get_dependents(experiment): def test_column_in_table(dataset): assert mut.is_column_in_table(dataset.conn, "runs", "run_id") - assert not mut.is_column_in_table(dataset.conn, "runs", "non-existing-column") + assert not mut.is_column_in_table(dataset.conn, "runs", + "non-existing-column") def test_run_exist(dataset): @@ -168,7 +168,6 @@ def test_get_last_experiment_no_experiments(empty_temp_db): def test_update_runs_description(dataset): - invalid_descs = ['{}', 'description'] for idesc in invalid_descs: @@ -212,16 +211,17 @@ def test_get_parameter_data(scalar_dataset): expected_names['param_3'] = ['param_0', 'param_1', 'param_2', 'param_3'] expected_shapes = {} - expected_shapes['param_3'] = [(10**3, )]*4 + expected_shapes['param_3'] = [(10 ** 3,)] * 4 expected_values = {} - expected_values['param_3'] = [np.arange(10000*a, 10000*a+1000) + expected_values['param_3'] = [np.arange(10000 * a, 10000 * a + 1000) for a in range(4)] verify_data_dict(data, input_names, expected_names, expected_shapes, expected_values) -def test_get_parameter_data_independent_parameters(standalone_parameters_dataset): +def test_get_parameter_data_independent_parameters( + standalone_parameters_dataset): ds = standalone_parameters_dataset params = mut.get_non_dependencies(ds.conn, ds.run_id) @@ -240,7 +240,7 @@ def test_get_parameter_data_independent_parameters(standalone_parameters_dataset expected_shapes = {} expected_shapes['param_1'] = [(10 ** 3,)] expected_shapes['param_2'] = [(10 ** 3,)] - expected_shapes['param_3'] = [(10**3, )]*2 + expected_shapes['param_3'] = [(10 ** 3,)] * 2 expected_values = {} expected_values['param_1'] = [np.arange(10000, 10000 + 1000)] @@ -270,3 +270,32 @@ def test_is_run_id_in_db(empty_temp_db): acquired_dict = mut.is_run_id_in_database(conn, *try_ids) assert expected_dict == acquired_dict + + +def test_atomic_creation(experiment): + def just_throw(*args): + raise RuntimeError("This breaks adding metadata") + + # first we patch add_meta_data to throw an exception + # if create_data is not atomic this would create a partial + # run in the db. Causing the next create_run to fail + with patch('qcodes.dataset.sqlite_base.add_meta_data', new=just_throw): + x = ParamSpec('x', 'numeric') + t = ParamSpec('t', 'numeric') + y = ParamSpec('y', 'numeric', depends_on=['x', 't']) + with pytest.raises(RuntimeError): + mut.create_run(experiment.conn, + experiment.exp_id, + name='testrun', + guid=generate_guid(), + parameters=[x, t, + y], + metadata={'a': 1}) + + mut.create_run(experiment.conn, + experiment.exp_id, + name='testrun', + guid=generate_guid(), + parameters=[x, t, + y], + metadata={'a': 1}) From b9e495c4569ad192ea416214f16d920ddaaa398d Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 15 Jan 2019 14:18:41 +0100 Subject: [PATCH 4/5] Improve test --- qcodes/tests/dataset/test_sqlite_base.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/qcodes/tests/dataset/test_sqlite_base.py b/qcodes/tests/dataset/test_sqlite_base.py index 7da436ba258..26a66447667 100644 --- a/qcodes/tests/dataset/test_sqlite_base.py +++ b/qcodes/tests/dataset/test_sqlite_base.py @@ -273,6 +273,10 @@ def test_is_run_id_in_db(empty_temp_db): def test_atomic_creation(experiment): + """" + Test that dataset creation is atomic. Test for + https://github.com/QCoDeS/Qcodes/issues/1444 + """ def just_throw(*args): raise RuntimeError("This breaks adding metadata") @@ -283,7 +287,8 @@ def just_throw(*args): x = ParamSpec('x', 'numeric') t = ParamSpec('t', 'numeric') y = ParamSpec('y', 'numeric', depends_on=['x', 't']) - with pytest.raises(RuntimeError): + with pytest.raises(RuntimeError, + match="Rolling back due to unhandled exception")as e: mut.create_run(experiment.conn, experiment.exp_id, name='testrun', @@ -291,7 +296,12 @@ def just_throw(*args): parameters=[x, t, y], metadata={'a': 1}) - + assert error_caused_by(e, "This breaks adding metadata") + # since we are starting from an empty database and the above transaction + # should be rolled back there should be no runs in the run table + runs = mut.transaction(experiment.conn, + 'SELECT run_id FROM runs').fetchall() + assert len(runs) == 0 mut.create_run(experiment.conn, experiment.exp_id, name='testrun', @@ -299,3 +309,7 @@ def just_throw(*args): parameters=[x, t, y], metadata={'a': 1}) + runs = mut.transaction(experiment.conn, + 'SELECT run_id FROM runs').fetchall() + assert len(runs) == 1 + From 33c8400e3595417bec6502913008f8c8384f08bc Mon Sep 17 00:00:00 2001 From: "Jens H. Nielsen" Date: Tue, 15 Jan 2019 14:55:57 +0100 Subject: [PATCH 5/5] Test the number of runs from a different connection This ensures that the runs are not only in memory but matches the db on disk --- qcodes/tests/dataset/test_sqlite_base.py | 25 ++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/qcodes/tests/dataset/test_sqlite_base.py b/qcodes/tests/dataset/test_sqlite_base.py index 26a66447667..256072160b8 100644 --- a/qcodes/tests/dataset/test_sqlite_base.py +++ b/qcodes/tests/dataset/test_sqlite_base.py @@ -4,6 +4,7 @@ from sqlite3 import OperationalError import tempfile import os +from contextlib import contextmanager import pytest import hypothesis.strategies as hst @@ -32,6 +33,17 @@ _unicode_categories = ('Lu', 'Ll', 'Lt', 'Lm', 'Lo', 'Nd', 'Pc', 'Pd', 'Zs') +@contextmanager +def shadow_conn(path_to_db: str): + """ + Simple context manager to create a connection for testing and + close it on exit + """ + conn = mut.connect(path_to_db) + yield conn + conn.close() + + def test_path_to_dbfile(): with tempfile.TemporaryDirectory() as tempdir: tempdb = os.path.join(tempdir, 'database.db') @@ -277,6 +289,7 @@ def test_atomic_creation(experiment): Test that dataset creation is atomic. Test for https://github.com/QCoDeS/Qcodes/issues/1444 """ + def just_throw(*args): raise RuntimeError("This breaks adding metadata") @@ -302,6 +315,13 @@ def just_throw(*args): runs = mut.transaction(experiment.conn, 'SELECT run_id FROM runs').fetchall() assert len(runs) == 0 + with shadow_conn(experiment.path_to_db) as new_conn: + runs = mut.transaction(new_conn, + 'SELECT run_id FROM runs').fetchall() + assert len(runs) == 0 + + # if the above was not correctly rolled back we + # expect the next creation of a run to fail mut.create_run(experiment.conn, experiment.exp_id, name='testrun', @@ -309,7 +329,12 @@ def just_throw(*args): parameters=[x, t, y], metadata={'a': 1}) + runs = mut.transaction(experiment.conn, 'SELECT run_id FROM runs').fetchall() assert len(runs) == 1 + with shadow_conn(experiment.path_to_db) as new_conn: + runs = mut.transaction(new_conn, + 'SELECT run_id FROM runs').fetchall() + assert len(runs) == 1