Skip to content

Use proper sync resolution in CI #1325

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Use proper sync resolution in CI #1325

wants to merge 2 commits into from
+7 −2

Conversation

@Kludex
Copy link
Member

@Kludex Kludex commented Aug 30, 2025

No description provided.

@Kludex Kludex requested review from a team and dsp-ant August 30, 2025 21:01
@Kludex Kludex mentioned this pull request Aug 30, 2025
Merged
4 tasks
@Kludex
Copy link
Member Author

Kludex commented Aug 30, 2025

@maxisbey Do you have time to debug this?

@maxisbey
Copy link
Contributor

maxisbey commented Sep 1, 2025

@maxisbey Do you have time to debug this?

Pretty sure it's because of this function here:

python-sdk/tests/client/test_config.py

Line 26 in 5439619

def test_command_execution(mock_config_path: Path):

It runs a uv command built here:

python-sdk/src/mcp/cli/claude.py

Line 67 in 4fee123

uv_path = get_uv_path()

This is then run without uv --frozen and without whatever dependency resolution is being set by the changes you made in this PR. So the unit test times out since it's re-installing a bunch of dependencies due to the different dependency version resolution uv is using when launched inside the unit test.

@felixweinberger felixweinberger added the needs more work Not ready to be merged yet, needs additional changes. label Sep 23, 2025
@felixweinberger
Copy link
Contributor

Will pick this up from @ihrpr

@felixweinberger felixweinberger added the needs maintainer action Potentially serious issue - needs proactive fix and maintainer attention label Sep 23, 2025
@felixweinberger felixweinberger self-assigned this Sep 23, 2025
@felixweinberger felixweinberger added this to the CI & Testing milestone Sep 30, 2025
@felixweinberger felixweinberger removed their assignment Oct 10, 2025
felixweinberger added a commit that referenced this pull request Oct 22, 2025
@felixweinberger
fix: use proper dependency resolution in CI
63363bf 
The CI was incorrectly using --frozen with both resolution strategies.
The --frozen flag should only be used with the highest resolution
strategy (which uses the lockfile), while lowest-direct resolution
needs to resolve dependencies dynamically using --resolution lowest-direct.

This change restructures the matrix to properly pass the correct flags
for each resolution strategy, fixing the CI pipeline.

Github-Issue: #1325
@felixweinberger felixweinberger mentioned this pull request Oct 22, 2025
Merged
9 tasks
felixweinberger added a commit that referenced this pull request Oct 22, 2025
@felixweinberger
fix: increase minimum versions for starlette and uvicorn
1b44d96 
Increase minimum versions for starlette and uvicorn to avoid
compatibility issues with lowest-direct dependency resolution.

The previous minimum versions (starlette>=0.27, uvicorn>=0.31.1)
allowed uvicorn 0.32.x which has a bug where uvicorn.loops.auto
module is missing the auto_loop_setup attribute, causing test
failures on Ubuntu runners.

Updated to:
- starlette>=0.40.0 (from 0.27)
- uvicorn>=0.33.0 (from 0.31.1)

This ensures lowest-direct resolution picks versions that are
compatible with the current codebase.

Github-Issue: #1325
felixweinberger added a commit that referenced this pull request Oct 22, 2025
@felixweinberger
fix: increase minimum versions for starlette and uvicorn
5274499 
Increase minimum versions for starlette and uvicorn to avoid
compatibility issues with lowest-direct dependency resolution.

The previous minimum versions (starlette>=0.27, uvicorn>=0.31.1)
allowed uvicorn 0.32.x which has a bug where uvicorn.loops.auto
module is missing the auto_loop_setup attribute, causing test
failures on Ubuntu runners.

Updated to:
- starlette>=0.40.0 (from 0.27)
- uvicorn>=0.33.0 (from 0.31.1)

This ensures lowest-direct resolution picks versions that are
compatible with the current codebase.

Github-Issue: #1325
felixweinberger added a commit that referenced this pull request Oct 22, 2025
@felixweinberger
fix: increase minimum versions for starlette and uvicorn
adb35bd 
Increase minimum versions for starlette and uvicorn to avoid
compatibility issues with lowest-direct dependency resolution.

The previous minimum versions (starlette>=0.27, uvicorn>=0.31.1)
allowed uvicorn 0.32.x which has a bug where uvicorn.loops.auto
module is missing the auto_loop_setup attribute, causing test
failures on Ubuntu runners.

Updated to:
- starlette>=0.40.0 (from 0.27)
- uvicorn>=0.33.0 (from 0.31.1)

This ensures lowest-direct resolution picks versions that are
compatible with the current codebase.

Github-Issue: #1325
felixweinberger added a commit that referenced this pull request Oct 22, 2025
@felixweinberger
fix: remove --frozen flag from uv run for lowest-direct resolution
feb95e1 
The --frozen flag should only be used with the highest resolution strategy
(which uses the lockfile). The lowest-direct resolution strategy needs to
resolve dependencies dynamically, so it cannot use --frozen.

This change adds a run-flags field to the matrix to conditionally apply
--frozen only to the highest resolution strategy, allowing lowest-direct
tests to run without the incompatible --frozen flag.

Github-Issue: #1325
felixweinberger added a commit that referenced this pull request Oct 22, 2025
@felixweinberger
fix: bump minimum anyio version to 4.10 to fix lowest-direct CI
59642be 
anyio 4.5.0 has an internal inconsistency where its asyncio backend
(_backends/_asyncio.py) tries to import RunFinishedError from
_core/_exceptions, but that exception doesn't exist in version 4.5.0.

This causes ImportError when tests try to set up fixtures with
--resolution lowest-direct, which installs anyio 4.5.0 (the previous
minimum version constraint).

Bumping to anyio>=4.10 ensures lowest-direct resolution installs a
version without this internal inconsistency.

Github-Issue: #1325
felixweinberger added a commit that referenced this pull request Oct 23, 2025
@felixweinberger
fix: refresh anyio package in lowest-direct resolution tests
7f67ada 
The CI was failing for lowest-direct tests due to cache corruption
where anyio 4.5.0 was being installed with mixed files from anyio 4.10.0.
This occurred because the cache was shared between the "highest" tests
(which install anyio 4.10.0 from the lockfile) and "lowest-direct" tests
(which resolve to anyio 4.5.0).

The symptom was that anyio's _backends/_asyncio.py tried to import
RunFinishedError (which only exists in anyio 4.11+), but the
_exceptions.py file was from anyio 4.5.0 (which doesn't have it),
causing ImportError.

This fix adds --refresh-package anyio to lowest-direct tests, forcing
anyio to be revalidated from PyPI while keeping the cache enabled for
other packages.

Github-Issue: #1325
felixweinberger added a commit that referenced this pull request Oct 23, 2025
@felixweinberger
fix: disable cache for lowest-direct resolution tests
03de4ae 
The CI was failing for lowest-direct tests due to cache corruption
where packages (specifically anyio) were installed with mixed files
from different versions. This occurred because the cache was shared
between "highest" tests (which use the lockfile) and "lowest-direct"
tests (which resolve to lowest compatible versions).

Initial attempt to use --refresh-package anyio did not resolve the
issue, indicating the cache corruption is at the wheel/file level
rather than just metadata.

This fix disables caching entirely for lowest-direct tests while
keeping it enabled for highest tests. This ensures lowest-direct
tests perform clean package resolution without cache interference,
which is the correct behavior for validating minimum dependencies.

Github-Issue: #1325
felixweinberger added a commit that referenced this pull request Oct 23, 2025
@felixweinberger
fix: remove --frozen flag from lowest-direct pytest runs
2fc9bc5 
Root cause identified: The --frozen flag was causing uv to validate
against the lockfile, which has anyio 4.10.0 (from highest resolution).
When running lowest-direct tests, we install anyio 4.5.0, but uv run
with --frozen was re-resolving or using the lockfile version, causing
the ImportError for RunFinishedError (which only exists in 4.11+).

Changes:
- lowest-direct tests now use: uv run --no-sync pytest
- highest tests still use: uv run --frozen --no-sync pytest
- Improved debugging to check venv directly without triggering re-resolution
- Added comparison tests between --no-sync and --frozen --no-sync

This ensures lowest-direct tests use the packages installed by
--resolution lowest-direct without interference from the lockfile.

Github-Issue: #1325
felixweinberger added a commit that referenced this pull request Oct 23, 2025
@felixweinberger
fix: run pytest directly from venv for lowest-direct tests
f4259f2 
The issue was that 'uv run' (even with --no-sync) was still interacting
with the project configuration and potentially re-resolving packages.

Solution: For lowest-direct tests, bypass uv run entirely and execute
pytest directly from the venv: .venv/bin/pytest

This ensures we use exactly what was installed by --resolution lowest-direct
without any interference from uv's project management or lockfile validation.

For highest tests, continue using 'uv run --frozen --no-sync pytest' which
validates against the lockfile as intended.

Github-Issue: #1325
felixweinberger added a commit that referenced this pull request Oct 23, 2025
@felixweinberger
refactor: simplify pytest execution to match PR #1325 approach
f77fc10 
Use uv run consistently for both resolution strategies, just setting
UV_RESOLUTION env var for lowest-direct to ensure test subprocesses
use the same resolution strategy.
@felixweinberger
Copy link
Contributor

Closing in favor of #1507

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dsp-ant dsp-ant Awaiting requested review from dsp-ant

Assignees

No one assigned

Labels

needs maintainer action Potentially serious issue - needs proactive fix and maintainer attention needs more work Not ready to be merged yet, needs additional changes.

Projects

None yet

Milestone

CI & Testing

Development

Successfully merging this pull request may close these issues.

4 participants

@Kludex @maxisbey @felixweinberger @dsp-ant