Skip to content

Lock uv dependencies for CI#562

Closed
davidt0x wants to merge 6 commits into
masterfrom
codex/migrate-ci-workflow-to-use-uv-projects
Closed

Lock uv dependencies for CI#562
davidt0x wants to merge 6 commits into
masterfrom
codex/migrate-ci-workflow-to-use-uv-projects

Conversation

@davidt0x

Copy link
Copy Markdown
Contributor

Summary

  • generate a comprehensive uv.lock that captures the project dependencies used by CI, including the development and example extras
  • run uv sync in CI with --frozen so the workflow relies on the checked-in lock file

Testing

  • uv sync --extra dev --extra matnormal --extra examples --frozen --dry-run

https://chatgpt.com/codex/tasks/task_e_68ffb8200b488320b9c9f1aed77f6fdc

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Updates CI to use uv for dependency management with a lockfile-driven environment, and aligns local development artifacts (like .venv/) with repo hygiene.

Changes:

  • Add .venv/ to .gitignore.
  • Switch CI from actions/setup-python + pip to astral-sh/setup-uv, uv python install, and uv sync with selected extras.
  • Update CI execution to run checks/tests/docs via uv run, and adjust the OS/Python test matrix.

Reviewed changes

Copilot reviewed 1 out of 3 changed files in this pull request and generated 3 comments.

File Description
.gitignore Ignore .venv/ created by uv/local tooling.
.github/workflows/ci.yml Migrate CI environment setup to uv and run checks/tests/docs through the locked environment; update matrix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/ci.yml
os: [ ubuntu-20.04, macos-latest, windows-latest ]
python-version: [ "3.9", "3.10", "3.11", "3.12" ]
os: [ ubuntu-22.04, macos-latest, windows-latest ]
python-version: [ "3.10", "3.11", "3.12" ]

Copilot AI Mar 30, 2026

Copy link

Choose a reason for hiding this comment

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

CI no longer tests Python 3.9, but pyproject.toml declares requires-python = ">=3.9" (and classifiers include 3.9). Either add 3.9 back to the matrix or update the project metadata/support policy to match, otherwise we may ship regressions for a still-supported version.

Suggested change
python-version: [ "3.10", "3.11", "3.12" ]
python-version: [ "3.9", "3.10", "3.11", "3.12" ]

Copilot uses AI. Check for mistakes.
Comment thread .github/workflows/ci.yml
Comment on lines +63 to +64
uv run ./run-tests.sh --sdist-mode
uv run bash -lc 'cd docs && make'

Copilot AI Mar 30, 2026

Copy link

Choose a reason for hiding this comment

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

run-tests.sh --sdist-mode disables the script’s editable-installation check (per the comment in run-tests.sh). Since uv.lock pins brainiak as an editable source, consider running ./run-tests.sh without --sdist-mode (or adjusting the script’s detection) so CI still verifies the intended install mode.

Copilot uses AI. Check for mistakes.
Comment thread .github/workflows/ci.yml
run: |
python3 -m pip install -U pip
- name: Synchronize project environment
run: uv sync --locked --extra dev --extra matnormal --extra examples

Copilot AI Mar 30, 2026

Copy link

Choose a reason for hiding this comment

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

The PR description says CI should run uv sync with --frozen to ensure the checked-in uv.lock is authoritative. This workflow currently uses uv sync --locked ..., which can still succeed even if the lockfile is out of date. Consider adding --frozen (optionally alongside --locked) so CI fails when pyproject.toml and uv.lock diverge.

Copilot uses AI. Check for mistakes.
Copilot AI added a commit that referenced this pull request Mar 30, 2026
- Incorporate uv-based CI workflow from codex/migrate-ci-workflow-to-use-uv-projects
- Add [tool.uv] no-binary-package = ['mpi4py'] to force source build
- Fix run-tests.sh to skip pip check when --sdist-mode is passed

Agent-Logs-Url: https://github.com/brainiak/brainiak/sessions/66d28b1c-6cb1-4922-9d90-d7f552a612b6

Co-authored-by: mihaic <165546+mihaic@users.noreply.github.com>
Copilot AI added a commit that referenced this pull request Mar 30, 2026
- pydicom: >=3.0.2 (path traversal CVE, was 2.4.4/3.0.1)
- pyOpenSSL: >=26.0.0 (DTLS buffer overflow, was 25.3.0)
- tornado: >=6.5.5 (DoS via multipart, was 6.5.2)
- wheel: >=0.46.2 (path traversal file permissions, was 0.45.1)
- requires-python bumped to >=3.10 (pydicom 3.x requires 3.10+;
  Python 3.9 was already removed from CI matrix in PR #562)
- Regenerate uv.lock with all patched versions

Agent-Logs-Url: https://github.com/brainiak/brainiak/sessions/66d28b1c-6cb1-4922-9d90-d7f552a612b6

Co-authored-by: mihaic <165546+mihaic@users.noreply.github.com>
mihaic added a commit that referenced this pull request Apr 2, 2026
Continuation of PR #562.

Also fix several CI failures.

## `fastsrm.py`: numpy 2.3 removed `_read_array_header`

`np.lib.format._read_array_header` was a private API removed in numpy
2.3. Replaced with the versioned public API and added a `with` context
manager (the bare `open()` was also causing `PermissionError: [WinError
32]` on Windows when the file stayed open during temp-dir cleanup):

```python
# Before
f = open(path, "rb")
version = np.lib.format.read_magic(f)
shape, fortran_order, dtype = np.lib.format._read_array_header(f, version)
f.close()

# After
with open(path, "rb") as f:
    version = np.lib.format.read_magic(f)
    if version == (1, 0):
        shape, fortran_order, dtype = np.lib.format.read_array_header_1_0(f)
    elif version == (2, 0):
        shape, fortran_order, dtype = np.lib.format.read_array_header_2_0(f)
    ...
```

## CI: MPI platform split — MPICH on Linux

OpenMPI on Ubuntu GitHub Actions runners hangs when `mpiexec` spawns
subprocesses. Split the `mpi4py/setup-mpi` step into OS-specific
variants: `mpich` on Linux, default (OpenMPI) on macOS, `msmpi` on
Windows (unchanged).

## `tests/conftest.py`: `pool_size` fixture logic bug causing EFA abort
+ mpiexec hang

The `pool_size` fixture was supposed to return `1` inside MPI
subprocesses to prevent `fork()`-after-`MPI_Init`. The guard was:

```python
if (multiprocessing.get_start_method() != "fork" and   # always False on Linux
        MPI.COMM_WORLD.Get_attr(MPI.APPNUM) is not None):
    return 1
```

On Linux, `get_start_method()` returns `"fork"` by default, so the
`APPNUM` check was never reached. With MPICH, `pool_size=2` was
returned, `multiprocessing.Pool(2)` forked inside MPI ranks, which
triggered the **libfabric EFA provider** on AWS runners to emit `"Your
job will now abort."` + SIGABRT. `mpiexec` then hung indefinitely →
every affected test hit the 120 s timeout.

Fix: drop the start-method guard so `APPNUM is not None` alone gates the
MPI-subprocess path:

```python
# Whenever inside an mpiexec subprocess, avoid fork() regardless of start method
if MPI.COMM_WORLD.Get_attr(MPI.APPNUM) is not None:
    return 1
```

Also removes the now-unused `import multiprocessing`.

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: mihaic <165546+mihaic@users.noreply.github.com>
Co-authored-by: David Turner <dmturner@princeton.edu>
@mihaic

mihaic commented Apr 3, 2026

Copy link
Copy Markdown
Member

Superseded by PR #564.

@mihaic mihaic closed this Apr 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants