Address GC with JPype ≥1.5.0

khaeru commented 10 months ago

Will close #463, #494, #501.

Also:

Add pytest-xdist to run tests in parallel on 2+ cores, as with other packages in our stack. This reduces the pytest run time for some jobs to <1 min.

One strange thing: the failures of the test_del_ts tests noted/discussed in #463 do not occur when using pytest-xdist; so those tests are all XPASS on Python ≤ 3.10 on all 3 OS.

How to review

Read the diff and note that the CI checks all pass.
Ensure the description of the JPype1 version dependency in the release notes is coherent.

PR checklist

[x] Continuous integration checks all ✅
[x] Add or expand tests; coverage checks both ✅
~Add, expand, or update documentation.~ N/A
[x] Update release notes.

codecov[bot] commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (d92cfcd) 98.8% compared to head (4f12382) 98.9%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #504 +/- ## ===================================== Coverage 98.8% 98.9% ===================================== Files 44 44 Lines 4747 4758 +11 ===================================== + Hits 4694 4706 +12 + Misses 53 52 -1 ``` | [Files](https://app.codecov.io/gh/iiasa/ixmp/pull/504?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa) | Coverage Δ | | |---|---|---| | [ixmp/backend/jdbc.py](https://app.codecov.io/gh/iiasa/ixmp/pull/504?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-aXhtcC9iYWNrZW5kL2pkYmMucHk=) | `97.2% <100.0%> (+<0.1%)` | :arrow_up: | | [ixmp/core/scenario.py](https://app.codecov.io/gh/iiasa/ixmp/pull/504?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-aXhtcC9jb3JlL3NjZW5hcmlvLnB5) | `98.7% <100.0%> (+0.4%)` | :arrow_up: | | [ixmp/tests/backend/test\_base.py](https://app.codecov.io/gh/iiasa/ixmp/pull/504?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-aXhtcC90ZXN0cy9iYWNrZW5kL3Rlc3RfYmFzZS5weQ==) | `98.9% <100.0%> (+<0.1%)` | :arrow_up: | | [ixmp/tests/backend/test\_jdbc.py](https://app.codecov.io/gh/iiasa/ixmp/pull/504?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-aXhtcC90ZXN0cy9iYWNrZW5kL3Rlc3RfamRiYy5weQ==) | `100.0% <100.0%> (ø)` | | | [ixmp/tests/core/test\_scenario.py](https://app.codecov.io/gh/iiasa/ixmp/pull/504?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-aXhtcC90ZXN0cy9jb3JlL3Rlc3Rfc2NlbmFyaW8ucHk=) | `100.0% <100.0%> (ø)` | | | [ixmp/tests/test\_cli.py](https://app.codecov.io/gh/iiasa/ixmp/pull/504?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-aXhtcC90ZXN0cy90ZXN0X2NsaS5weQ==) | `100.0% <100.0%> (ø)` | | | [ixmp/tests/test\_model.py](https://app.codecov.io/gh/iiasa/ixmp/pull/504?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-aXhtcC90ZXN0cy90ZXN0X21vZGVsLnB5) | `100.0% <ø> (ø)` | | | [ixmp/tests/test\_util.py](https://app.codecov.io/gh/iiasa/ixmp/pull/504?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iiasa#diff-aXhtcC90ZXN0cy90ZXN0X3V0aWwucHk=) | `100.0% <ø> (ø)` | |

glatterf42 commented 10 months ago

While looking at these logs, I noticed:

XPASS ixmp/tests/core/test_scenario.py::TestScenario::test_add_par[args2-kwargs2] Length mismatch between keys and values

Taking a closer look, I found

key_or_data = ["new-york", "chicago"]
value = [100, 200, 300]

# This creates as many tuples as possible, stopping when key_or_data runs out
zip(key_or_data, value)
# So there was never nan data in pandas, thus not triggering the ValueError we expect for the test

glatterf42 commented 10 months ago

Update: this only seems to work for python > 3.9, thus some tests are failing. I'll think about as elegant a fix as possible.

khaeru commented 10 months ago

I would suggest to use itertools.zip_longest from the standard library, and keep the if block as it was.

Note that the construction:

try:
    func()
except ValueError as e:
    raise ValueError("Helpful message about specific condition X") from e

…can only be used if we know for certain that only condition X will cause func() to raise ValueError. If there are 2 or more ways it could raise the same class of exception, then the except clause will inappropriately swallow/hide conditions Y or Z from the user, and actually give them an incorrect message about what they've done wrong.

glatterf42 commented 10 months ago

That's a good suggestion. In theory, though, we could also not reraise our own unique exception since zip(..., strict=True) will raise a ValueError already.

glatterf42 commented 10 months ago

It's supposed to, at least. On my system, however:

>>> zip(key_or_data, value, strict=True)
<zip object at 0x7fbe4169f9c0>
>>> list(zip(key_or_data, value, strict=True))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: zip() argument 2 is longer than argument 1

So itertools.zip_longest() it is.

iiasa / ixmp