Skip to content

docs: match documented dev setup and test commands to CI - #1407

Open
roshaninfordham wants to merge 2 commits into
roboflow:developfrom
roshaninfordham:fix/contributing-cpu-test-commands
Open

docs: match documented dev setup and test commands to CI#1407
roshaninfordham wants to merge 2 commits into
roboflow:developfrom
roshaninfordham:fix/contributing-cpu-test-commands

Conversation

@roshaninfordham

Copy link
Copy Markdown

Description

.github/CONTRIBUTING.md names the CI workflows as the source of truth for test commands, but the commands it shows had drifted from them in three places. Following the file as written does not produce a working local environment.

On a clean macOS checkout:

following CONTRIBUTING.md as written : 118 failed, 2529 passed, 210 errors
following ci-tests-cpu.yml           : 4406 passed, 67 skipped, 0 failed

Type of Change

  • 📝 Documentation update

Motivation and Context

Three separate drifts, each of which produces failures that look like a broken repository rather than a setup problem:

  1. Install. uv sync --all-groups installs dependency groups but no extras, so everything importing the training dependencies errors on import. The obvious next thing to try, uv sync --all-extras, hard-errors instead: Extras 'coreml' and 'executorch' are incompatible with the declared conflicts. ci-tests-cpu.yml uses uv pip install -e ".[train,augment,cli,visual]" --group tests with UV_TORCH_BACKEND=cpu, and ci-tests-gpu.yml already carries a comment explaining why uv pip rather than uv sync — that reasoning was never surfaced in CONTRIBUTING.

  2. CPU markers. The documented expression is -m "not gpu". CI uses -m "not gpu and not coco17 and not e2e_coreml and not e2e_executorch and not e2e_roboflow and not xla and not tpu". The extra exclusions are what keep suites needing the COCO dataset, a Roboflow API key, or an accelerator from running locally.

  3. GPU markers. The documented expression is -m gpu; CI uses -m "gpu and not e2e_tensorrt".

Changes Made

  • Replace the install snippet with the extras and groups the CPU test job installs, and record why uv pip install is used rather than uv sync.
  • Update both marker expressions to match ci-tests-cpu.yml and ci-tests-gpu.yml.
  • Add one short paragraph naming what the extra markers exclude and why, so the list is maintainable rather than magic.

Testing

  • I have tested this code locally
  • All new and existing tests pass

Documentation-only change; verified by running the commands as written after the edit.

uv pip install -e ".[train,augment,cli,visual,coreml]" --group tests   # UV_TORCH_BACKEND=cpu, macOS
uv run --no-sync pytest src/ tests/ -n 2 \
  -m "not gpu and not coco17 and not e2e_coreml and not e2e_executorch and not e2e_roboflow and not xla and not tpu" \
  --ignore=tests/run_smoke_all_models.py --ignore=tests/legacy/test_checkpoint_compat.py --timeout=240

→ 4406 passed, 67 skipped in 115s.

pre-commit run --all-files — 18 hooks pass. The mypy local hook fails in my environment with No module named mypy; it fails identically on a stashed clean tree, so it is a local environment artifact rather than something this change introduces, and this change touches no Python.

Additional Notes

I have kept uv sync --group docs and uv sync --group build for the docs and build workflows, where no extras are needed.

The macOS coreml extra is mentioned in a comment rather than added to the command, matching how ci-tests-cpu.yml conditionally appends it.

`.github/CONTRIBUTING.md` names the CI workflows as the source of truth, but
the commands it shows had drifted from them in three places:

- the install step used `uv sync --all-groups`, which installs no extras, so
  the suite errored on importing the training dependencies
- the CPU marker expression was `-m "not gpu"` against CI's much wider
  exclusion of coco17, e2e_coreml, e2e_executorch, e2e_roboflow, xla and tpu
- the GPU marker expression omitted `not e2e_tensorrt`

Following the file as written produced 118 failed and 210 errors on a clean
macOS checkout; the CI commands produce 4406 passed. Also record why
`uv pip install` is used rather than `uv sync`, since `uv sync --all-extras`
fails outright on the declared coreml/executorch conflict.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7b2f4946ae

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread .github/CONTRIBUTING.md
uv sync --all-groups
# Install the extras and groups the CPU test job uses (add ,coreml on macOS).
# UV_TORCH_BACKEND=cpu keeps this from pulling a CUDA build of PyTorch.
UV_TORCH_BACKEND=cpu uv pip install -e ".[train,augment,cli,visual]" --group tests

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Add the missing uv environment creation step

In a fresh checkout, this command now fails before dependency resolution because uv pip install requires an existing virtual environment; verified with the documented command plus --dry-run, which returns No virtual environment found; run uv venv to create an environment. The previous uv sync flow created .venv automatically, so new contributors following these setup steps will not get a working test environment unless the guide first creates/activates a venv (or uses another explicit target).

Useful? React with 👍 / 👎.

uv pip install requires an existing virtualenv and does not create one, so
the rewritten setup steps failed on a fresh checkout with

    error: No virtual environment found; run `uv venv` to create an environment

The uv sync flow they replaced created .venv implicitly, which is why the
step was not needed before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant