From c50181378ed3eae433c13314bb61325d672c4579 Mon Sep 17 00:00:00 2001 From: Paul Natsuo Kishimoto Date: Thu, 27 Jul 2023 15:13:19 +0200 Subject: [PATCH 1/7] Add .utils.discard_on_error() --- ixmp/utils/__init__.py | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/ixmp/utils/__init__.py b/ixmp/utils/__init__.py index 98f67058e..d3c693b27 100644 --- a/ixmp/utils/__init__.py +++ b/ixmp/utils/__init__.py @@ -1,8 +1,9 @@ import logging import re import sys +from contextlib import contextmanager from pathlib import Path -from typing import Dict, Iterable, Iterator, List, Tuple +from typing import TYPE_CHECKING, Dict, Iterable, Iterator, List, Tuple from urllib.parse import urlparse from warnings import warn @@ -11,6 +12,9 @@ from ixmp.backend import ItemType +if TYPE_CHECKING: + from ixmp import TimeSeries + log = logging.getLogger(__name__) # Globally accessible logger. @@ -159,6 +163,28 @@ def diff(a, b, filters=None) -> Iterator[Tuple[str, pd.DataFrame]]: break +@contextmanager +def discard_on_error(ts: "TimeSeries"): + """Discard changes to `ts` on any exception and close the database connection. + + For :mod:`JDBCBackend`, this can avoid leaving a scenario in the "locked" state in + the database. + """ + mp = ts.platform + try: + yield + except Exception as e: + log.info(f"Avoid locking {ts!r} before raising {str(e).splitlines()[0]!r}") + try: + ts.discard_changes() + log.info("Discard scenario changes") + except Exception: + pass + mp.close_db() + log.info("Close database connection") + raise + + def maybe_check_out(timeseries, state=None): """Check out `timeseries` depending on `state`. From 96db8d403dc57c3bf2ba48ec5f6106a7ddc134a1 Mon Sep 17 00:00:00 2001 From: Paul Natsuo Kishimoto Date: Thu, 27 Jul 2023 15:15:08 +0200 Subject: [PATCH 2/7] =?UTF-8?q?Add=20TimeSeries.transact(=E2=80=A6,=20disc?= =?UTF-8?q?ard=5Fon=5Ferror=3D=E2=80=A6)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ixmp/core/timeseries.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/ixmp/core/timeseries.py b/ixmp/core/timeseries.py index 9b82f64b4..3bbb2fd25 100644 --- a/ixmp/core/timeseries.py +++ b/ixmp/core/timeseries.py @@ -1,5 +1,5 @@ import logging -from contextlib import contextmanager +from contextlib import contextmanager, nullcontext from os import PathLike from pathlib import Path from typing import Any, Dict, Optional, Sequence, Tuple, Union @@ -203,7 +203,9 @@ def discard_changes(self) -> None: self._backend("discard_changes") @contextmanager - def transact(self, message: str = "", condition: bool = True): + def transact( + self, message: str = "", condition: bool = True, discard_on_error: bool = False + ): """Context manager to wrap code in a 'transaction'. If `condition` is :obj:`True`, the TimeSeries (or :class:`.Scenario`) is @@ -221,10 +223,15 @@ def transact(self, message: str = "", condition: bool = True): >>> # Changes to `ts` have been committed """ # TODO implement __enter__ and __exit__ to allow simpler "with ts: …" + from ixmp.utils import discard_on_error as discard_on_error_cm + if condition: maybe_check_out(self) try: - yield + # Use the discard_on_error context manager (cm) if the parameter of the same + # name is True + with discard_on_error_cm(self) if discard_on_error else nullcontext(): + yield finally: maybe_commit(self, condition, message) From 78fc02217193c86fd314e487b42f5eb39aa399bf Mon Sep 17 00:00:00 2001 From: Paul Natsuo Kishimoto Date: Tue, 8 Aug 2023 11:50:55 +0200 Subject: [PATCH 3/7] Prefix "ixmp-" to pytest CLI options --- ixmp/testing/__init__.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/ixmp/testing/__init__.py b/ixmp/testing/__init__.py index 3a48a9ee1..d7e58a2f1 100644 --- a/ixmp/testing/__init__.py +++ b/ixmp/testing/__init__.py @@ -86,7 +86,12 @@ def pytest_addoption(parser): """Add the ``--user-config`` command-line option to pytest.""" parser.addoption( - "--user-config", + "--ixmp-jvm-mem", + action="store", + help="Heap space to allocated for the JDBCBackend/JVM.", + ) + parser.addoption( + "--ixmp-user-config", action="store_true", help="Use the user's existing config/'local' platform.", ) @@ -96,7 +101,7 @@ def pytest_sessionstart(session): """Unset any configuration read from the user's directory.""" from ixmp.backend import jdbc - if not session.config.option.user_config: + if not session.config.option.ixmp_user_config: ixmp_config.clear() # Further clear an automatic reference to the user's home directory. See fixture # tmp_env below. @@ -164,7 +169,7 @@ def tmp_env(pytestconfig, tmp_path_factory): base_temp = tmp_path_factory.getbasetemp() os.environ["IXMP_DATA"] = str(base_temp) - if not pytestconfig.option.user_config: + if not pytestconfig.option.ixmp_user_config: # Set the path for the default/local platform in the test directory localdb = base_temp.joinpath("localdb", "default") ixmp_config.values["platform"]["local"]["path"] = localdb From 977337dc77e4dadbecbb8336df1ae94f34d69e12 Mon Sep 17 00:00:00 2001 From: Paul Natsuo Kishimoto Date: Tue, 8 Aug 2023 12:26:53 +0200 Subject: [PATCH 4/7] Test discard_on_error() --- ixmp/tests/test_utils.py | 41 ++++++++++++++++++++++++++++++++++++++++ ixmp/utils/__init__.py | 5 ++++- 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/ixmp/tests/test_utils.py b/ixmp/tests/test_utils.py index 0518fa116..74c945b62 100644 --- a/ixmp/tests/test_utils.py +++ b/ixmp/tests/test_utils.py @@ -134,6 +134,47 @@ def test_diff_items(test_mp): pass # No check of the contents +def test_discard_on_error(caplog, test_mp): + caplog.set_level(logging.INFO, "ixmp.utils") + + # Create a test scenario, checked-in state + s = make_dantzig(test_mp) + url = s.url + + # Some actions that don't trigger exceptions + assert dict(value=90, unit="USD/km") == s.scalar("f") + s.check_out() + s.change_scalar("f", 100, "USD/km") + + # Catch the deliberately-raised exception so that the test passes + with pytest.raises(KeyError): + # Trigger KeyError and the discard_on_error() behaviour + with utils.discard_on_error(s): + s.add_par("d", pd.DataFrame([["foo", "bar", 1.0, "kg"]])) + + # Exception was caught and logged + assert caplog.messages[-3].startswith("Avoid locking ") + assert [ + "Discard scenario changes", + "Close database connection", + ] == caplog.messages[-2:] + + # Re-load the mp and the scenario + with pytest.raises(RuntimeError): + # Fails because the connection to test_mp was closed by discard_on_error() + s2 = Scenario(test_mp, **utils.parse_url(url)[1]) + + # Reopen the connection + test_mp.open_db() + + # Now the scenario can be reloaded + s2 = Scenario(test_mp, **utils.parse_url(url)[1]) + assert s2 is not s # Different object instance than above + + # Data modification above was discarded by discard_on_error() + assert dict(value=90, unit="USD/km") == s.scalar("f") + + def test_filtered(): df = pd.DataFrame() assert df is utils.filtered(df, filters=None) diff --git a/ixmp/utils/__init__.py b/ixmp/utils/__init__.py index d3c693b27..126c2e97b 100644 --- a/ixmp/utils/__init__.py +++ b/ixmp/utils/__init__.py @@ -174,7 +174,10 @@ def discard_on_error(ts: "TimeSeries"): try: yield except Exception as e: - log.info(f"Avoid locking {ts!r} before raising {str(e).splitlines()[0]!r}") + log.info( + f"Avoid locking {ts!r} before raising {e.__class__.__name__}: " + + str(e).splitlines()[0].strip('"') + ) try: ts.discard_changes() log.info("Discard scenario changes") From 4bac8274701ce678d7edcb9bd75f20d6e7753839 Mon Sep 17 00:00:00 2001 From: Paul Natsuo Kishimoto Date: Tue, 8 Aug 2023 12:47:41 +0200 Subject: [PATCH 5/7] =?UTF-8?q?Test=20Timeseries.transact(=E2=80=A6,=20dis?= =?UTF-8?q?card=5Fon=5Ferror=3DTrue)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ixmp/tests/core/test_timeseries.py | 30 ++++++++++++++++++++++++++++++ ixmp/utils/__init__.py | 2 +- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/ixmp/tests/core/test_timeseries.py b/ixmp/tests/core/test_timeseries.py index 7c7e1ecb1..3f768b63c 100644 --- a/ixmp/tests/core/test_timeseries.py +++ b/ixmp/tests/core/test_timeseries.py @@ -1,3 +1,5 @@ +import logging +import re from datetime import datetime, timedelta import pandas as pd @@ -313,6 +315,34 @@ def test_remove(self, mp, ts, commit): # Result is empty assert ts.timeseries().empty + def test_transact_discard(self, caplog, mp, ts): + caplog.set_level(logging.INFO, "ixmp.utils") + + df = expected(DATA[2050], ts) + + ts.add_timeseries(DATA[2050]) + ts.commit("") + + # Catch the deliberately-raised exception so that the test passes + with pytest.raises(AttributeError): + with ts.transact(discard_on_error=True): + # Remove a single data point; this operation will not be committed + ts.remove_timeseries(df[df.year == 2010]) + + # Trigger AttributeError + ts.foo_bar() + + # Reopen the database connection + mp.open_db() + + # Exception was caught and logged + assert caplog.messages[-4].startswith("Avoid locking ") + assert re.match("Discard (timeseries|scenario) changes", caplog.messages[-3]) + assert "Close database connection" == caplog.messages[-2] + + # Data are unchanged + assert_frame_equal(expected(DATA[2050], ts), ts.timeseries()) + # Geodata def test_add_geodata(self, ts): diff --git a/ixmp/utils/__init__.py b/ixmp/utils/__init__.py index 126c2e97b..4140bbf69 100644 --- a/ixmp/utils/__init__.py +++ b/ixmp/utils/__init__.py @@ -180,7 +180,7 @@ def discard_on_error(ts: "TimeSeries"): ) try: ts.discard_changes() - log.info("Discard scenario changes") + log.info(f"Discard {ts.__class__.__name__.lower()} changes") except Exception: pass mp.close_db() From f33c557a6b474f8f8e93b288917fea6750609efb Mon Sep 17 00:00:00 2001 From: Paul Natsuo Kishimoto Date: Tue, 8 Aug 2023 12:49:58 +0200 Subject: [PATCH 6/7] Exclude TYPE_CHECKING blocks from coverage --- ixmp/utils/__init__.py | 2 +- ixmp/utils/sphinx_linkcode_github.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ixmp/utils/__init__.py b/ixmp/utils/__init__.py index 4140bbf69..e558712b1 100644 --- a/ixmp/utils/__init__.py +++ b/ixmp/utils/__init__.py @@ -12,7 +12,7 @@ from ixmp.backend import ItemType -if TYPE_CHECKING: +if TYPE_CHECKING: # pragma: no cover from ixmp import TimeSeries log = logging.getLogger(__name__) diff --git a/ixmp/utils/sphinx_linkcode_github.py b/ixmp/utils/sphinx_linkcode_github.py index 2307749d8..46869a5de 100644 --- a/ixmp/utils/sphinx_linkcode_github.py +++ b/ixmp/utils/sphinx_linkcode_github.py @@ -10,7 +10,7 @@ from sphinx.util import logging -if TYPE_CHECKING: +if TYPE_CHECKING: # pragma: no cover import sphinx.application log = logging.getLogger(__name__) From 246be1bd3c88ba34a8193f864b037299697f7766 Mon Sep 17 00:00:00 2001 From: Paul Natsuo Kishimoto Date: Tue, 8 Aug 2023 13:10:53 +0200 Subject: [PATCH 7/7] Add #488 to docs and release notes --- RELEASE_NOTES.rst | 6 ++++-- doc/api.rst | 1 + ixmp/core/timeseries.py | 20 ++++++++++++++++---- ixmp/utils/__init__.py | 30 +++++++++++++++++++++++++----- 4 files changed, 46 insertions(+), 11 deletions(-) diff --git a/RELEASE_NOTES.rst b/RELEASE_NOTES.rst index 773297461..b7573fa42 100644 --- a/RELEASE_NOTES.rst +++ b/RELEASE_NOTES.rst @@ -1,9 +1,11 @@ -.. Next release -.. ============ +Next release +============ .. All changes .. ----------- +- New :func:`.utils.discard_on_error` and matching argument to :meth:`.TimeSeries.transact` to avoid locking :class:`.TimeSeries` / :class:`.Scenario` on failed operations with :class:`.JDBCBackend` (:pull:`488`). + .. _v3.7.0: v3.7.0 (2023-05-17) diff --git a/doc/api.rst b/doc/api.rst index 818a2cb8d..f51f06835 100644 --- a/doc/api.rst +++ b/doc/api.rst @@ -205,6 +205,7 @@ Utilities .. autosummary:: diff + discard_on_error format_scenario_list maybe_check_out maybe_commit diff --git a/ixmp/core/timeseries.py b/ixmp/core/timeseries.py index 3bbb2fd25..be2c2462b 100644 --- a/ixmp/core/timeseries.py +++ b/ixmp/core/timeseries.py @@ -208,10 +208,22 @@ def transact( ): """Context manager to wrap code in a 'transaction'. - If `condition` is :obj:`True`, the TimeSeries (or :class:`.Scenario`) is - checked out *before* the block begins. When the block ends, the object is - committed with `message`. If `condition` is :obj:`False`, nothing occurs before - or after the block. + Parameters + ---------- + message : str + Commit message to use, if any commit is performed. + condition : bool + If :obj:`True` (the default): + + - Before entering the code block, the TimeSeries (or :class:`.Scenario`) is + checked out. + - On exiting the code block normally (without an exception), changes are + committed with `message`. + + If :obj:`False`, nothing occurs on entry or exit. + discard_on_error : bool + If :obj:`True` (default :obj:`False`), then the anti-locking behaviour of + :func:`.discard_on_error` also applies to any exception raised in the block. Example ------- diff --git a/ixmp/utils/__init__.py b/ixmp/utils/__init__.py index e558712b1..7361a723f 100644 --- a/ixmp/utils/__init__.py +++ b/ixmp/utils/__init__.py @@ -165,10 +165,26 @@ def diff(a, b, filters=None) -> Iterator[Tuple[str, pd.DataFrame]]: @contextmanager def discard_on_error(ts: "TimeSeries"): - """Discard changes to `ts` on any exception and close the database connection. + """Context manager to discard changes to `ts` and close the DB on any exception. - For :mod:`JDBCBackend`, this can avoid leaving a scenario in the "locked" state in - the database. + For :mod:`JDBCBackend`, this can avoid leaving `ts` in a "locked" state in the + database. + + Examples + -------- + >>> mp = ixmp.Platform() + >>> s = ixmp.Scenario(mp, ...) + >>> with discard_on_error(s): + ... s.add_par(...) # Any code + ... s.not_a_method() # Code that raises some exception + + Before the the exception in the final line is raised (and possibly handled by + surrounding code): + + - Any changes—for example, here changes due to the call to :meth:`.add_par`—are + discarded/not committed; + - ``s`` is guaranteed to be in a non-locked state; and + - :meth:`.close_db` is called on ``mp``. """ mp = ts.platform try: @@ -178,13 +194,17 @@ def discard_on_error(ts: "TimeSeries"): f"Avoid locking {ts!r} before raising {e.__class__.__name__}: " + str(e).splitlines()[0].strip('"') ) + try: ts.discard_changes() + except Exception: # pragma: no cover + pass # Some exception trying to discard changes() + else: log.info(f"Discard {ts.__class__.__name__.lower()} changes") - except Exception: - pass + mp.close_db() log.info("Close database connection") + raise