Skip to content

Commit

Permalink
Merge pull request #504 from iiasa/issue/463
Browse files Browse the repository at this point in the history
Address GC with JPype ≥1.5.0
  • Loading branch information
khaeru authored Jan 9, 2024
2 parents d92cfcd + 4f12382 commit 41f8a96
Show file tree
Hide file tree
Showing 11 changed files with 72 additions and 35 deletions.
20 changes: 9 additions & 11 deletions .github/workflows/pytest.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ jobs:
- "3.8" # Earliest version supported by ixmp
- "3.9"
- "3.10"
- "3.11" # Latest supported by ixmp
# - "3.12" # Pending JPype support; see iiasa/ixmp#501
- "3.11"
- "3.12" # Latest supported by ixmp

# commented: force a specific version of pandas, for e.g. pre-release
# testing
Expand Down Expand Up @@ -92,10 +92,6 @@ jobs:
run: echo "RETICULATE_PYTHON=$pythonLocation" >> $GITHUB_ENV
shell: bash

- name: Work around https://bugs.launchpad.net/lxml/+bug/2035206
if: matrix.python-version == '3.8' && matrix.os == 'macos-latest'
run: pip install "lxml == 4.9.2"

- name: Install Python package and dependencies
# [docs] contains [tests], which contains [report,tutorial]
run: |
Expand All @@ -104,10 +100,6 @@ jobs:
# commented: use with "pandas-version" in the matrix, above
# pip install --upgrade pandas${{ matrix.pandas-version }}
- name: TEMPORARY Work around iiasa/ixmp#463
if: matrix.python-version != '3.11'
run: pip install "JPype1 != 1.4.1"

- name: Install R dependencies and tutorial requirements
run: |
install.packages(c("remotes", "Rcpp"))
Expand All @@ -125,7 +117,13 @@ jobs:
shell: Rscript {0}

- name: Run test suite using pytest
run: pytest ixmp -m "not performance" --verbose -rA --cov-report=xml --color=yes
run: |
pytest ixmp \
-m "not performance" \
--color=yes -rA --verbose \
--cov-report=xml \
--numprocesses=auto
shell: bash

- name: Upload test coverage to Codecov.io
uses: codecov/codecov-action@v3
Expand Down
6 changes: 5 additions & 1 deletion RELEASE_NOTES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ Code that imports from the old locations will continue to work, but will raise :
All changes
-----------

- Support for Python 3.7 is dropped (:pull:`492`).
- :mod:`ixmp` is tested and compatible with `Python 3.12 <https://www.python.org/downloads/release/python-3120/>`__ (:pull:`504`).
- Support for Python 3.7 is dropped (:pull:`492`), as it has reached end-of-life.
- Rename :mod:`ixmp.report` and :mod:`ixmp.util` (:pull:`500`).
- New reporting operators :func:`.from_url`, :func:`.get_ts`, and :func:`.remove_ts` (:pull:`500`).
- New CLI command :program:`ixmp platform copy` and :doc:`CLI documentation <cli>` (:pull:`500`).
Expand All @@ -28,6 +29,9 @@ All changes
- When a :class:`.GAMSModel` is solved with an LP status of 5 (optimal, but with infeasibilities after unscaling), :class:`.JDBCBackend` would attempt to read the output GDX file and fail, leading to an uninformative error message (:issue:`98`).
Now :class:`.ModelError` is raised describing the situation.
- Improved type hinting for static typing of code that uses :mod:`ixmp` (:issue:`465`, :pull:`500`).
- :mod:`ixmp` requires JPype1 1.4.0 or earlier, for Python 3.10 and earlier (:pull:`504`).
With JPype1 1.4.1 and later, memory management in :class:`.CachingBackend` may not function as intended (:issue:`463`), which could lead to high memory use where many, large :class:`.Scenario` objects are created and used in a single Python program.
(For Python 3.11 and later, any version of JPype1 from the prior minimum (1.2.1) to the latest is supported.)

.. _v3.7.0:

Expand Down
19 changes: 13 additions & 6 deletions ixmp/backend/jdbc.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,7 @@ def _raise_jexception(exc, msg="unhandled Java exception: "):
"""Convert Java/JPype exceptions to ordinary Python RuntimeError."""
# Try to re-raise as a ValueError for bad model or scenario name
arg = exc.args[0] if isinstance(exc.args[0], str) else ""
match = re.search(r"getting '([^']*)' in table '([^']*)'", arg)
if match:
if match := re.search(r"getting '([^']*)' in table '([^']*)'", arg):
param = match.group(2).lower()
if param in {"model", "scenario"}:
raise ValueError(f"{param}={repr(match.group(1))}") from None
Expand Down Expand Up @@ -301,7 +300,7 @@ def __init__(self, jvmargs=None, **kwargs):
f"{e}\nCheck that dependencies of ixmp.jar are "
f"included in {Path(__file__).parents[2] / 'lib'}"
)
except jpype.JException as e: # pragma: no cover
except java.Exception as e: # pragma: no cover
# Handle Java exceptions
jclass = e.__class__.__name__
if jclass.endswith("HikariPool.PoolInitializationException"):
Expand Down Expand Up @@ -708,10 +707,18 @@ def get(self, ts):
# either getTimeSeries or getScenario
method = getattr(self.jobj, "get" + ts.__class__.__name__)

# Re-raise as a ValueError for bad model or scenario name, or other
with _handle_jexception():
# Re-raise as a ValueError for bad model or scenario name, or other with
# with _handle_jexception():
try:
# Either the 2- or 3- argument form, depending on args
jobj = method(*args)
except SystemError:
# JPype 1.5.0 with Python 3.12: "<built-in method __subclasscheck__ of
# _jpype._JClass object at …> returned a result with an exception set"
# At least transmute to a ValueError
raise ValueError("model, scenario, or version not found")
except BaseException as e:
_raise_jexception(e)

self._index_and_set_attrs(jobj, ts)

Expand Down Expand Up @@ -938,7 +945,7 @@ def init_item(self, s, type, name, idx_sets, idx_names):
# by Backend, so don't return here
try:
func(name, idx_sets, idx_names)
except jpype.JException as e:
except java.Exception as e:
if "already exists" in e.args[0]:
raise ValueError(f"{repr(name)} already exists")
else:
Expand Down
2 changes: 1 addition & 1 deletion ixmp/core/scenario.py
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,7 @@ def add_par(
# Multiple values
values = value

data = pd.DataFrame(zip(keys, values), columns=["key", "value"])
data = pd.DataFrame(zip_longest(keys, values), columns=["key", "value"])
if data.isna().any(axis=None):
raise ValueError("Length mismatch between keys and values")

Expand Down
11 changes: 10 additions & 1 deletion ixmp/tests/backend/test_base.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import sys
from pathlib import Path

import pytest
Expand Down Expand Up @@ -69,7 +70,9 @@ class BE2(Backend):
def test_class():
# An incomplete Backend subclass can't be instantiated
with pytest.raises(
TypeError, match="Can't instantiate abstract class BE1 with abstract methods"
TypeError,
match="Can't instantiate abstract class BE1 with(out an implementation for)? "
"abstract methods",
):
BE1()

Expand Down Expand Up @@ -145,6 +148,12 @@ def test_del_ts(self, test_mp):
# Cache size has increased
assert cache_size_pre + 1 == len(backend._cache)

# JPype ≥ 1.4.1 with Python ≤ 3.10 produces danging traceback/frame references
# to `s` that prevent it being GC'd at "del s" below. See
# https://github.com/iiasa/ixmp/issues/463 and test_jdbc.test_del_ts
if sys.version_info.minor <= 10:
s.__del__() # Force deletion of cached objects associated with `s`

# Delete the object; associated cache is freed
del s

Expand Down
7 changes: 7 additions & 0 deletions ixmp/tests/backend/test_jdbc.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import logging
import os
import platform
import sys
from sys import getrefcount
from typing import Tuple

Expand Down Expand Up @@ -370,6 +371,12 @@ def test_verbose_exception(test_mp, exception_verbose_true):
assert "at.ac.iiasa.ixmp.Platform.getScenario" in exc_msg


@pytest.mark.xfail(
condition=sys.version_info.minor <= 10,
raises=AssertionError,
# See also test_base.TestCachingBackend.test_del_ts
reason="https://github.com/iiasa/ixmp/issues/463",
)
def test_del_ts():
mp = ixmp.Platform(
backend="jdbc",
Expand Down
12 changes: 9 additions & 3 deletions ixmp/tests/core/test_scenario.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import re
import sys

import numpy.testing as npt
import pandas as pd
Expand Down Expand Up @@ -93,10 +94,15 @@ def test_from_url(self, mp, caplog):
with pytest.raises(Exception, match=expected):
scen, mp = ixmp.Scenario.from_url(url + "#10000", errors="raise")

# Giving an invalid scenario with errors='warn' raises an exception
# Giving an invalid scenario with errors='warn' causes a message to be logged
msg = (
"ValueError: scenario='Hitchhikerfoo'\n"
f"when loading Scenario from url: {repr(url + 'foo')}"
"ValueError: "
+ (
"scenario='Hitchhikerfoo'"
if sys.version_info.minor != 12
else "model, scenario, or version not found"
)
+ f"\nwhen loading Scenario from url: {repr(url + 'foo')}"
)
with assert_logs(caplog, msg):
scen, mp = ixmp.Scenario.from_url(url + "foo")
Expand Down
8 changes: 7 additions & 1 deletion ixmp/tests/test_cli.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import re
import sys
from pathlib import Path

import pandas as pd
Expand Down Expand Up @@ -403,7 +404,12 @@ def test_solve(ixmp_cli, test_mp):
]
result = ixmp_cli.invoke(cmd)
assert result.exit_code == 1, result.output
assert "Error: model='non-existing'" in result.output
exp = (
"='non-existing'"
if sys.version_info.minor != 12
else ", scenario, or version not found"
)
assert f"Error: model{exp}" in result.output

result = ixmp_cli.invoke([f"--url=ixmp://{test_mp.name}/foo/bar", "solve"])
assert UsageError.exit_code == result.exit_code, result.output
Expand Down
4 changes: 3 additions & 1 deletion ixmp/tests/test_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ class M1(Model):
pass

with pytest.raises(
TypeError, match="Can't instantiate abstract class M1 " "with abstract methods"
TypeError,
match="Can't instantiate abstract class M1 with(out an implementation for)? "
"abstract methods",
):
M1()

Expand Down
10 changes: 5 additions & 5 deletions ixmp/tests/test_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,17 +92,17 @@ def test_diff_data(test_mp):
# Expected results
exp_b = pd.DataFrame(
[
["chicago", 300.0, "cases", np.NaN, None, "left_only"],
["new-york", np.NaN, None, 325.0, "cases", "right_only"],
["chicago", 300.0, "cases", np.nan, np.nan, "left_only"],
["new-york", np.nan, np.nan, 325.0, "cases", "right_only"],
["topeka", 275.0, "cases", 275.0, "cases", "both"],
],
columns="j value_a unit_a value_b unit_b _merge".split(),
)
exp_d = pd.DataFrame(
[
["san-diego", "chicago", np.NaN, None, 1.8, "km", "right_only"],
["san-diego", "new-york", np.NaN, None, 2.5, "km", "right_only"],
["san-diego", "topeka", np.NaN, None, 1.4, "km", "right_only"],
["san-diego", "chicago", np.nan, np.nan, 1.8, "km", "right_only"],
["san-diego", "new-york", np.nan, np.nan, 2.5, "km", "right_only"],
["san-diego", "topeka", np.nan, np.nan, 1.4, "km", "right_only"],
["seattle", "chicago", 1.7, "km", 123.4, "km", "both"],
["seattle", "new-york", 2.5, "km", 123.4, "km", "both"],
["seattle", "topeka", 1.8, "km", 123.4, "km", "both"],
Expand Down
8 changes: 3 additions & 5 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ classifiers = [
"Programming Language :: Python :: 3.9",
"Programming Language :: Python :: 3.10",
"Programming Language :: Python :: 3.11",
"Programming Language :: Python :: 3.12",
"Programming Language :: R",
"Topic :: Scientific/Engineering",
"Topic :: Scientific/Engineering :: Information Analysis",
Expand All @@ -33,6 +34,7 @@ dependencies = [
"click",
"genno >= 1.16",
"JPype1 >= 1.2.1",
"JPype1 <= 1.4.0; python_version < '3.11'",
"openpyxl",
"pandas >= 1.2",
"pint",
Expand Down Expand Up @@ -65,6 +67,7 @@ tests = [
"pytest-benchmark",
"pytest-cov",
"pytest-rerunfailures",
"pytest-xdist",
]

[project.scripts]
Expand All @@ -77,11 +80,6 @@ exclude_also = [
]
omit = ["ixmp/util/sphinx_linkcode_github.py"]

[tool.isort]
profile = "black"

[tool.mypy]

[[tool.mypy.overrides]]
# Packages/modules for which no type hints are available.
module = [
Expand Down

0 comments on commit 41f8a96

Please sign in to comment.