add lowest-direct dependency resolution

[abhardwaj73]

Summary

Major changes:

• feature 1: add dependency instead of only python versions
• fix 1:

        version:
          - { python: "3.9", resolution: highest, extras: strict }
          - { python: "3.10", resolution: lowest-direct, extras: strict }
          - { python: "3.11", resolution: highest, extras: non-strict }
          - { python: "3.12", resolution: lowest-direct, extras: non-strict }

instead of: python-version: ['3.9', '3.10', '3.11']

similar to in materialsproject/jobflow#640 (comment)

Todos

If this is not the right approach, @rkingsbury please let me know

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.09%. Comparing base (65beb17) to head (1c57f26).
Report is 3 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #163   +/-   ##
=======================================
  Coverage   83.09%   83.09%           
=======================================
  Files           9        9           
  Lines        1479     1479           
  Branches      319      319           
=======================================
  Hits         1229     1229           
  Misses        214      214           
  Partials       36       36           

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

[rkingsbury]

Closes #157

[rkingsbury]

Thanks @abhardwaj73 ! Studying the pymatgen PR more closely, I was about to say that you need to pass the resolution argument into the matrix call here, e.g.

name: Install dependencies
        run: |
          pip install uv
          uv pip install '.[${{ matrix.version.extras }}]' --system --resolution=${{ matrix.version.resolution }}

But then I realized that pymatgen is using uv, not pip to install dependencies, and that package is what's receiving the argument (not pip - pip actually doesn't have a resolution option).

I was not familiar with uv, but it appears to be a much faster alternative to pip, much like how ruff is a much faster linter than flake8 or pycodestyle. It could be beneficial to use in unit tests because it will make them run faster (by installing deps faster). Plus obviously make it possible to use these different resolution strategies.

Can you update this to install uv, similar to the pymatgen workflow, and then pass it the resolution argument?

[abhardwaj73]

Added uv and resolution parameter. I'm not sure if we need to keep tests in uv pip install so have added commented line for it.

[rkingsbury]

Thanks @abhardwaj73 ! I see what you're asking; I think a small change is still needed.

So in this line

uv pip install '.[${{ matrix.version.extras }}]' --system --resolution=${{ matrix.version.resolution }}

the bit '.[${{ matrix.version.extras }}]' is going to replace matrix.version.extras with the value of extras from the matrix defined above:

matrix:
        version:
          - { python: "3.9", resolution: highest, extras: strict }
          - { python: "3.10", resolution: lowest-direct, extras: strict }
          - { python: "3.11", resolution: highest, extras: non-strict }
          - { python: "3.12", resolution: lowest-direct, extras: non-strict }

Much like python-version: ${{ matrix.version.python }}${{ matrix.dev }} pulls the version.python variable.

So since extras=strict, '.[${{ matrix.version.extras }}]' will turn into '.[strict]' when the action runs. You are probably familiar with this syntax - it's how you specify optional dependency groups with pip install.

In pyEQL, we don't have a strict dependency group, but we do have testing and docs (se pyproject.toml). So for testing, you should set extras: testing for every version in the matrix. Then the command should equate to uv pip install ".[testing]"

On a related note, I don't think the variable dev is defined anywhere in the test matrix, so I expect ${{ matrix.dev }} will cause an error unless we remove it.

Please adjust as you see fit and then I'll merge. Ultimately we'll just have to see what happens when post-merge runs (since, by definition, it doesn't run when a PR is open like this), and then make adjustments as needed after that.

[abhardwaj73]

Actually, I didn't sync my code with the last PR (Sean's log edit)
Should I sync it, update the files, and then push?

[abhardwaj73]

On a related note, I don't think the variable dev is defined anywhere in the test matrix, so I expect ${{ matrix.dev }} will cause an error unless we remove it.

Removing this line was not causing the tests to pass, I think it is important.

[rkingsbury]

Actually, I didn't sync my code with the last PR (Sean's log edit) Should I sync it, update the files, and then push?

Either way is OK, since you are working on different parts of the code.

Removing this line was not causing the tests to pass, I think it is important.

I see why you thought that. I looked at the test failure and it's actually caused by something different (which I need to figure out how to fix). TL;DR - I recently added parallelization to the unit tests, and this seems to be causing intermittent, non-reproducible problems in the cases where multiple threads are trying to access the same file. If I manually re-run the failed test, it usually works.

Anyway, there's no way you would know this; I just have seen it enough times that it's familiar. This is a good reminder to investigate.

I'll merge and let's see what happens!

[rkingsbury]

OK, not working yet - https://github.com/KingsburyLab/pyEQL/actions/runs/10293990190. there's another instance of python-version (Line 48) that needs to be changed to version.python