WIP - Refactor PHREEQC's _setup_ppsol

[SuixiongTay]

Summary

This PR refactors the PhreeqcEOS and Phreeqc2026EOS engines to reduce duplicated code while preserving the engine specific methods. This PR addresses #354 .

• PhreeqcEOS inherits the PHREEQC setup logic from Phreeqc2026EOS, unless replaced by methods specific for that engine.
• The duplicated _setup_ppsol in PhreeqcEOS solution input is reduced to _ppsol_dict_input.
• Activity coefficient is reduced to the helper functions, including _get_activity, and _get_molality.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.49%. Comparing base (6a2279c) to head (5b9b4e2).
⚠️ Report is 23 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #397      +/-   ##
==========================================
+ Coverage   86.32%   86.49%   +0.16%     
==========================================
  Files          14       14              
  Lines        1865     1851      -14     
  Branches      328      319       -9     
==========================================
- Hits         1610     1601       -9     
+ Misses        209      207       -2     
+ Partials       46       43       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[SuixiongTay]

Hi @rkingsbury , may I have your review on this draft?

test_salt_with_equilibration passes consistently on pytest but there appears to be a threading fail in the CI. Should we be changing the CI worker setup instead? Thanks!

============================ short test summary info ============================
SKIPPED [6] test_salt_matching.py:306: got empty parameter set ('cutoff', 'volume'), function test_should_not_return_salts_with_concentration_below_cutoff at /home/st2591/pyEQL_shaun/pyEQL/tests/test_salt_matching.py:305
SKIPPED [6] test_salt_matching.py:322: got empty parameter set ('cutoff', 'volume'), function test_should_return_all_salts_with_concentrations_above_cutoff at /home/st2591/pyEQL_shaun/pyEQL/tests/test_salt_matching.py:321
================== 657 passed, 12 skipped in 203.32s (0:03:23) ==================

[rkingsbury]

Nice work @SuixiongTay - elegant refactoring. I too am confused by the test failures - I've never seen runners crash like this.

I cloned your branch and ran it locally and all tests pass - as it sounds like they do for you. It's also interesting that our CI testing workflows all pass, but not testing-pinned. That makes me think the failures possibly reflect a bug in an underlying dependency that got updated (remember, testing-pinned uses exact versions of all dependencies, testing uses the latest versions allowed by our pyproject.toml file).

I'm ready to merge this PR, but I want to resolve the test failure first. Let me look into it - no action needed on your end for now.

@vineetbansal - do you have any ideas on what could be happening here?

[vineetbansal]

Strange! Let me take a look at what's happening here tomorrow.

[vineetbansal]

@SuixiongTay - I think a genuine bug has been introduced in this PR, and it will be worthwhile testing it out on different platforms on the CI to see if it only shows up on a mac or on multiple platforms. Can you merge latest changes from main to your branch and then disable fail-fast in the CI so that the PR runs on all platforms?

It's probably some C-level garbage-collection stuff. If we observe the crash on Linux as well (hopefully) then I can clone this branch locally and see what might be happening.

[SuixiongTay]

Thanks @vineetbansal with your guidance on this, feel free to let me know how I can be of help!

[vineetbansal]

Thanks @SuixiongTay - now we see that some tests are failing anyway (these would fail locally for you as well), even outside of the segmentation fault issue. Perhaps you want to fix these so the only issues that remain are the ones you can't explain?

Also, I'm not sure why the testing-pinned tests were cancelled. Perhaps let them run (after fixing the failing tests).

[rkingsbury]

@vineetbansal @SuixiongTay the one test failures was introduced by a marge with main - one key , recently added line got deleted when it shouldn't have. I've restored it so the normal test workflow should pass now.

[vineetbansal]

@SuixiongTay - I meant you will have to add fail-fast: false on both workflows (not just change true->false in one), since failing fast is the default behavior.

[vineetbansal]

@SuixiongTay - so it looks like it might not be a reproducible problem on all platforms after all. Can you confirm that the tests pass for you if you use dependencies from requirements.txt on your mac?

I'm inclined to think that pytest-xdist is messing with an import that was moved to the module level in this PR:

from pyEQL.phreeqc import PHRQSol  # noqa: PLC0415

but it would be nice to confirm this on a regular (i.e. non-CI) mac before going that route.