Fix CI Divergence (Mostly mypy)
#1293
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| name: docs | ||
|
|
||
| on: | ||
| pull_request: | ||
| branches: | ||
| - main | ||
| - dev | ||
| paths: | ||
| - 'docs/**' | ||
| workflow_dispatch: | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| jobs: | ||
| check_links: | ||
| runs-on: [self-hosted, Linux] | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Python | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: '3.12' | ||
|
Member
Author
There was a problem hiding this comment. In this workflow (documentation checks) the version shouldn't matter |
||
|
|
||
| - name: Run doclinks # Note: doesn't need uv python, just needs some python to run the script | ||
|
Contributor
There was a problem hiding this comment. git-commit hooks run this, so this is already executed by our
Member
Author
There was a problem hiding this comment. I was thinking we would change it where this is the only thing that would run on .md file edits (to make it really fast to merge doc changes). If we keep code cleanup though, yeah I guess it would be good to remove it. I guess there are those whitespace checks in pre-commit that still effect docs
Contributor
There was a problem hiding this comment. not sure if missing something, why add complexity for speed gains if pre commit stuff runs in less then a second? |
||
| run: | | ||
| python -m dimos.utils.docs.doclinks docs/ | ||
|
|
||
| - uses: leshy/md-babel-py@main | ||
| with: | ||
| files: 'docs/**/*.md' | ||
| fail-on-change: true | ||
| # maybe later we can enforce python, for now just do diagram langs | ||
|
Member
Author
There was a problem hiding this comment. Previously we weren't checking diagram generation AFAIK, this adds that |
||
| args: --lang asymptote,pikchr,openscad,diagon | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| name: test-fast | ||
|
|
||
| on: | ||
| push: | ||
| branches: | ||
| - main | ||
| - dev | ||
| paths: | ||
| - 'pyproject.toml' | ||
| - 'setup.py' | ||
| - 'uv.lock' | ||
| - 'dimos/**' | ||
| - '!dimos/**/*.md' | ||
| workflow_dispatch: | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| jobs: | ||
| pytest_fast: | ||
| strategy: | ||
| matrix: | ||
| os: [ubuntu-24.04, macos-15, ubuntu-24.04-arm] | ||
|
Member
Author
There was a problem hiding this comment. Discussion needed: do we want to host our own macos / linux-arm testers? (or keep this as-is and run on github)
jeff-hykin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| runs-on: ${{ matrix.os }} | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup uv | ||
| uses: astral-sh/setup-uv@v4 | ||
| with: | ||
| enable-cache: true | ||
| python-version: "3.10" | ||
| cache-suffix: ${{ runner.os }}-${{ runner.arch }}-py3.10 | ||
|
|
||
| - name: Install System dependencies | ||
| run: | | ||
| # these lines should stay exactly in sync with docs/development/README.md | ||
|
Member
Author
There was a problem hiding this comment. This is an important change: we should have something on CI that matches our docs, and this new test does that |
||
| if [ "$OSTYPE" = "linux-gnu" ]; then | ||
| sudo apt-get update | ||
| sudo apt-get install -y curl g++ portaudio19-dev git-lfs libturbojpeg python3-dev pre-commit | ||
| # On macOS (12.6 or newer) | ||
| elif [ "$(uname)" = "Darwin" ]; then | ||
| # install homebrew if not installed | ||
| ! command -v brew && /bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)" | ||
| # install dependencies | ||
| brew install gnu-sed gcc portaudio git-lfs libjpeg-turbo python pre-commit | ||
| fi | ||
|
|
||
| - name: Install and run tests | ||
| run: | | ||
| bin/ci-fast | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| #!/usr/bin/env bash | ||
|
Member
Author
There was a problem hiding this comment. This file is the overlap between local and CI: this file gets executed by CI and is executed locally (when running |
||
|
|
||
| # | ||
| # note this command is used by CI directly, and also is used by bin/pr-check | ||
| # this is intentional: to keep them in sync | ||
| # if something should only be done locally, change bin/pr-check | ||
| # if something should only be done on CI, change .github/workflows/fast-test.yml | ||
| # | ||
| set -euo pipefail | ||
|
|
||
| echo "Ensuring pip versions are correct" | ||
| time uv sync --frozen --extra misc --extra visualization --extra agents --extra web --extra perception --extra unitree --extra manipulation --extra cpu --extra dev --extra sim --extra drone --extra base | ||
|
Contributor
There was a problem hiding this comment. I'm not sure this should be here. In CI, the packages should have been installed in the python step, no? And locally this removes packages. (I do
Member
Author
There was a problem hiding this comment. No, AFAIK the github uv python action doesn install with --frozen which is what is needed to prevent mypy from diverging. Just run the workflow and you'll see this command does cause installs. Running the command when everything is already installed is almost instant (no reinstall, just performs a check). As for locally, its still basically necessary. I agree uninstallation isnt ideal but I don't think theres a way to say "keep all the features I selected (dont uninstall anything) but use the frozen versions from UV". If you dont want to run the command thats fine but IMO we should at least have one command that exactly replicates CI
Member
Author
There was a problem hiding this comment. I can try to make a "use frozen versions of everything but dont uninstall anything" command. If its not too hacky maybe that would be useful |
||
|
|
||
| echo "Running pre-commit checks" | ||
| time pre-commit run --all-files | ||
|
|
||
| echo "Running mypy checks" | ||
| time uv run mypy --python-version 3.10 dimos | ||
| time uv run mypy --python-version 3.12 dimos | ||
|
Member
Author
There was a problem hiding this comment. This could be faster by excuting in parallel, but I opted for simplicity. No cache ~5min on github runners, so its pretty fast |
||
|
|
||
| echo "# " | ||
| echo "# " | ||
| echo "# pytest fast" | ||
| echo "# " | ||
| echo "# " | ||
| time uv run bin/pytest-fast | ||
|
Contributor
There was a problem hiding this comment. pytest-fast has this: You probably just want to run
Member
Author
There was a problem hiding this comment. My thought was, if we edit pytest-fast (to exclude something, or add something) we would want that change to be in sync with CI. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| #!/usr/bin/env bash | ||
|
|
||
| # | ||
| # this file is supposed to mimic CI as closesly as possible, the main difference is it doesn't detect changes it just runs everything | ||
| # | ||
| set -euo pipefail | ||
|
|
||
| echo "If you get permission errors try: echo YOUR_GITHUB_AUTH_TOKEN | docker login ghcr.io -u GITHUB_USER --password-stdin" | ||
|
|
||
| run_in_image() { | ||
| local image="$1" | ||
| local cmd="$2" | ||
|
|
||
| if [ "$(uname)" = "Darwin" ]; then | ||
| docker run --rm -it -v "$PWD:/workspace" -w /workspace --platform linux/amd64 -e CI=1 "ghcr.io/dimensionalos/${image}:dev" /entrypoint.sh bash -lc "$cmd" | ||
| else | ||
| docker run --rm -it -v "$PWD:/workspace" -w /workspace -e CI=1 "ghcr.io/dimensionalos/${image}:dev" /entrypoint.sh bash -lc "$cmd" | ||
| fi | ||
| } | ||
|
|
||
| echo "Running checks equivalent to .github/workflows/docker.yml test jobs..." | ||
|
|
||
| echo | ||
| echo "== ros-dev: pytest && pytest -m ros ==" | ||
| run_in_image "ros-dev" "pytest && pytest -m ros" | ||
|
|
||
| echo | ||
| echo "== dev: pytest ==" | ||
| run_in_image "dev" "pytest" | ||
|
|
||
| echo | ||
| echo "== dev: pytest -m heavy ==" | ||
| run_in_image "dev" "pytest -m heavy" | ||
|
|
||
| echo | ||
| echo "== dev: pytest -m lcm ==" | ||
| run_in_image "dev" "pytest -m lcm" | ||
|
|
||
| echo | ||
| echo "== dev: pytest -m integration ==" | ||
| run_in_image "dev" "pytest -m integration" | ||
|
Contributor
There was a problem hiding this comment. If you're running all of them sequentially, you might as well run a single command: |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
leshy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" | ||
| cd "$REPO_ROOT" | ||
|
|
||
| # if md-babel-py doesnt exist | ||
| if [ -z "$(command -v "md-babel-py")" ]; then | ||
| # if nix doesnt exist | ||
| if [ -z "$(command -v "nix")" ]; then | ||
| echo "Error: md-babel-py required for running gen-diagrams." >&2 | ||
| echo " Either install nix or install md-babel-py" >&2 | ||
| echo " https://github.com/leshy/md-babel-py" >&2 | ||
| exit 1 | ||
| # use nix if local command doesn't exist | ||
| else | ||
| md-babel-py() { | ||
| nix run github:leshy/md-babel-py -- "$@" | ||
| } | ||
| fi | ||
| fi | ||
|
|
||
| diagram_langs="asymptote,pikchr,openscad,diagon" | ||
| if [[ "$#" -gt 0 ]]; then | ||
| for arg in "$@"; do | ||
| md-babel-py run "$arg" --lang "$diagram_langs" | ||
| done | ||
| else | ||
| while IFS= read -r file; do | ||
| md-babel-py run "$file" --lang "$diagram_langs" | ||
| done < <(find ./docs -type f -name '*.md' | sort) | ||
| fi | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| #!/usr/bin/env python3 | ||
|
Member
Author
There was a problem hiding this comment. This is what everyone should run locally before making a pull request.
A lot of good enhancements could be added, but I wanted to start simple
Contributor
There was a problem hiding this comment. Why just the fast tests? Shouldn't people run all the tests? (The non-fast ones are more commonly broken on dev especially since a few of them don't run in CI at all.) |
||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import os | ||
| from pathlib import Path | ||
| import subprocess | ||
|
|
||
| repo_root = Path(__file__).resolve().parent.parent | ||
| os.chdir(repo_root) | ||
|
|
||
|
|
||
| def main() -> int: | ||
| # 1. name check (Note: doesn't throw even on fail) | ||
| subprocess.run(["bin/pr-name-check"], check=False, text=True) | ||
|
|
||
| # 2. check if only markdown changes | ||
| changed_files = get_changed_files() | ||
| for each in changed_files: | ||
| print(f"Changed file: {each}") | ||
| if all(changed_file.endswith(".md") for changed_file in changed_files): | ||
| print("All detected changes are Markdown files; just running doc tests.") | ||
| subprocess.run( | ||
| ["python", "-m", "dimos.utils.docs.doclinks", "docs/"], check=True, text=True | ||
| ) | ||
| return 0 | ||
|
|
||
| # 3. run ci checks (uv install, mypy, ruff, pytest) | ||
| return subprocess.run(["bin/ci-fast"], text=True).returncode | ||
|
|
||
|
|
||
| def get_changed_files() -> set[str]: | ||
| def has_ref(ref: str) -> bool: | ||
| result = subprocess.run( | ||
| ["git", "show-ref", "--verify", "--quiet", ref], | ||
| text=True, | ||
| capture_output=True, | ||
| ) | ||
| return result.returncode == 0 | ||
|
|
||
| if has_ref("refs/heads/dev"): | ||
| base_ref = "dev" | ||
| elif has_ref("refs/remotes/origin/dev"): | ||
| base_ref = "origin/dev" | ||
| else: | ||
| print("Could not find 'dev' or 'origin/dev' to compare changes against.") | ||
| raise SystemExit(1) | ||
|
|
||
| def run_lines(cmd: list[str]) -> list[str]: | ||
| return [ | ||
| line | ||
| for line in subprocess.run( | ||
| cmd, check=True, text=True, capture_output=True | ||
| ).stdout.splitlines() | ||
| if line.strip() | ||
| ] | ||
|
|
||
| changed_files: set[str] = set() | ||
| changed_files.update(run_lines(["git", "diff", "--name-only", f"{base_ref}...HEAD"])) | ||
| changed_files.update(run_lines(["git", "diff", "--name-only"])) | ||
| changed_files.update(run_lines(["git", "diff", "--name-only", "--cached"])) | ||
| changed_files.update(run_lines(["git", "ls-files", "--others", "--exclude-standard"])) | ||
| return changed_files | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| raise SystemExit(main()) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a nessary change, based on: https://github.com/dimensionalOS/dimos/actions/runs/22160016061/job/64077594237?pr=1293