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.

Sign up for GitHub

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

add lowest-direct dependency resolution #163

Merged
merged 5 commits into from
Aug 8, 2024
Merged

add lowest-direct dependency resolution #163

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3b1142b
add lowest-direct dependency resolution
abhardwaj73 Aug 5, 2024
dab5187
Added uv instead of pip for resolution
abhardwaj73 Aug 7, 2024
94bd146
Commented line for possible replacement of resulution line
abhardwaj73 Aug 7, 2024
f96063d
updated extras in python version and removed dev
abhardwaj73 Aug 7, 2024
1c57f26
added matrix.dev to check if test pass
abhardwaj73 Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions .github/workflows/post-process.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,11 @@ jobs:
strategy:
max-parallel: 6
matrix:
python-version: ["3.9", "3.10", "3.11", "3.12"]
version:
- { python: "3.9", resolution: highest, extras: testing }
- { python: "3.10", resolution: lowest-direct, extras: testing }
- { python: "3.11", resolution: highest, extras: testing }
- { python: "3.12", resolution: lowest-direct, extras: testing }
os:
- ubuntu-latest
- macos-latest
Expand All @@ -48,11 +52,12 @@ jobs:
- name: Setup Python
uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}${{ matrix.dev }}
python-version: ${{ matrix.version.python }}${{ matrix.dev }}
Copy link
Member

Choose a reason for hiding this comment

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

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?

- name: Install test requirements
run: |
python -m pip install --upgrade pip
pip install -e ".[testing]"
pip install uv
uv pip install '.[${{ matrix.version.extras }}]' --system --resolution=${{ matrix.version.resolution }}
- name: Run tests
run: |
pytest -n auto --cov=src/pyEQL --cov-report=xml
Expand Down