Skip to content
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

Address GC with JPype ≥1.5.0 #504

Merged
merged 14 commits into from
Jan 9, 2024
Merged

Address GC with JPype ≥1.5.0 #504

merged 14 commits into from
Jan 9, 2024

Conversation

khaeru
Copy link
Member

@khaeru khaeru commented Jan 8, 2024

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 Adjust for JPype1 v1.4.1 #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

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

@khaeru khaeru added bug backend.jdbc Interaction with ixmp_source via JDBCBackend & JPype labels Jan 8, 2024
@khaeru khaeru self-assigned this Jan 8, 2024
@khaeru khaeru linked an issue Jan 8, 2024 that may be closed by this pull request
@khaeru khaeru linked an issue Jan 8, 2024 that may be closed by this pull request
Copy link

codecov bot commented Jan 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@          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 Coverage Δ
ixmp/backend/jdbc.py 97.2% <100.0%> (+<0.1%) ⬆️
ixmp/core/scenario.py 98.7% <100.0%> (+0.4%) ⬆️
ixmp/tests/backend/test_base.py 98.9% <100.0%> (+<0.1%) ⬆️
ixmp/tests/backend/test_jdbc.py 100.0% <100.0%> (ø)
ixmp/tests/core/test_scenario.py 100.0% <100.0%> (ø)
ixmp/tests/test_cli.py 100.0% <100.0%> (ø)
ixmp/tests/test_model.py 100.0% <ø> (ø)
ixmp/tests/test_util.py 100.0% <ø> (ø)

@khaeru khaeru mentioned this pull request Jan 8, 2024
3 tasks
@glatterf42
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

khaeru commented Jan 9, 2024

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
Copy link
Member

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
Copy link
Member

glatterf42 commented Jan 9, 2024

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.

Copy link
Member

@glatterf42 glatterf42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me now, thanks :)

@khaeru khaeru merged commit 41f8a96 into main Jan 9, 2024
21 checks passed
@khaeru khaeru added this to the 3.8 milestone Jan 9, 2024
@glatterf42 glatterf42 deleted the issue/463 branch January 9, 2024 14:39
@glatterf42 glatterf42 mentioned this pull request Mar 18, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend.jdbc Interaction with ixmp_source via JDBCBackend & JPype bug
Projects
None yet
2 participants