Conversation
This reverts commit dd90ef4.
…s into jeff/fix/ci
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…s into jeff/fix/gen-diagrams
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
| @@ -0,0 +1,32 @@ | |||
| #!/usr/bin/env bash | |||
| set -euo pipefail | |||
There was a problem hiding this comment.
Merge #1291 (very small) first and this diff will go away
| @@ -0,0 +1,99 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
This is what everyone should run locally before making a pull request.
It
- checks branch name (you can ignore the error and keep going)
- runs only doclink checks if only markdown is edited
- runs
bin/ci-fastif more than just markdown is edited
A lot of good enhancements could be added, but I wanted to start simple
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.)
| @@ -0,0 +1,68 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
I don't care which branch-naming-scheme we use, I just implemented a check for the one that is listed on our wiki <name>/<type>/<description>
Right now its not a forceful/blocking check it'll just yell at you if it doesnt match
| # Install dependencies with UV (10-100x faster than pip) | ||
| RUN uv pip install --upgrade 'pip>=24' 'setuptools>=70' 'wheel' 'packaging>=24' && \ | ||
| uv pip install '.[misc,cpu,sim,drone,unitree,web,perception,visualization]' | ||
| 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 |
There was a problem hiding this comment.
Most Important Change
--frozen instead of "close enough" install
There was a problem hiding this comment.
An issue I see here is that this forces us to always run with uv run .... This should be fixable by adding some of the variables which activating gives you. This could be enough:
ENV VIRTUAL_ENV=/app/.venv
ENV PATH="/app/.venv/bin:$PATH"
Another issue that worries me is that uv sync always installs in .venv. The previous command installed in the system python. By installing in /app it means we can't mount our local dir as /app because it would use the .venv we have locally instead of the one in the docker image.
Not sure what the solution is here. (Maybe copy pyproject.toml to /app-deps and install from that dir so the packages are installed outside /app?
| - If it only matters to people who contribute to dimos (like this doc), put them in `docs/development` | ||
| - Otherwise put them in `docs/usage` | ||
| 2. Run `bin/gen_diagrams` to generate the svg's for your diagrams. We use [pikchr](https://pikchr.org/home/doc/trunk/doc/userman.md) as a diagram language. | ||
| 2. Run `bin/gen-diagrams` to generate the svg's for your diagrams. We use [mermaid](https://mermaid.js.org/intro/) (no generation needed) and [pikchr](https://pikchr.org/home/doc/trunk/doc/userman.md) as diagrams languages. |
There was a problem hiding this comment.
Merge #1291 (very small) first and this diff will go away
| cmd: "MYPYPATH=/opt/ros/humble/lib/python3.10/site-packages mypy dimos" | ||
| dev-image: ros-dev:${{ (needs.check-changes.outputs.python == 'true' || needs.check-changes.outputs.dev == 'true' || needs.check-changes.outputs.ros == 'true') && needs.ros-dev.result == 'success' && needs.check-changes.outputs.branch-tag || 'dev' }} | ||
|
|
||
| # Run module tests directly to avoid pytest forking issues |
There was a problem hiding this comment.
If we need to restore old code, lets use git as it was meant to be (instead of having giant blocks of commented out code
| with: | ||
| cmd: "pytest -m heavy" | ||
| cmd: "uv run pytest -m heavy" | ||
| dev-image: dev:${{ (needs.check-changes.outputs.python == 'true' || needs.check-changes.outputs.dev == 'true') && needs.dev.result == 'success' && needs.check-changes.outputs.branch-tag || 'dev' }} |
There was a problem hiding this comment.
I think this is a nessary change, based on: https://github.com/dimensionalOS/dimos/actions/runs/22160016061/job/64077594237?pr=1293
| # Planning (Drake) | ||
| "drake>=1.40.0; platform_machine != 'aarch64'", | ||
| "drake==1.45.0; sys_platform == 'darwin' and platform_machine != 'aarch64'", | ||
| "drake>=1.40.0; sys_platform != 'darwin' and platform_machine != 'aarch64'", |
There was a problem hiding this comment.
This is both a fix for MacOS and Arm. On MacOS drake 1.45.0 is needed, on x86 arm anything >1.40 is okay.
The drake team says they're interested in supporting Arm linux so hopefully that'll come soon
| cpu = [ | ||
| # CPU inference backends | ||
| "onnxruntime", | ||
| "onnxruntime<1.24; python_version < '3.11'", |
There was a problem hiding this comment.
CI caught this which is great: it was impossible for python 3.10 to have the version of onnx we had specified in the lock file. This guard fixes that
| from setuptools import find_packages, setup | ||
|
|
||
|
|
||
| def python_is_macos_universal_binary(executable: str | None = None) -> bool: |
There was a problem hiding this comment.
without this change macos-15 github uv python runner can't uv sync
|
@greptile-apps please re-review. Edit your original message if you are able to |
|
|
||
| echo | ||
| echo "== dev: pytest -m integration ==" | ||
| run_in_image "dev" "pytest -m integration" |
There was a problem hiding this comment.
If you're running all of them sequentially, you might as well run a single command:
run_in_image "dev" "-m 'not (tool or module or neverending or mujoco)'"
Co-authored-by: Paul Nechifor <paul@nechifor.net>
Co-authored-by: Paul Nechifor <paul@nechifor.net>
.github/workflows/docker.yml
Outdated
| secrets: inherit | ||
| with: | ||
| cmd: "pytest && pytest -m ros" # run tests that depend on ros as well | ||
| cmd: "uv run pytest && uv run pytest -m ros" # run tests that depend on ros as well |
There was a problem hiding this comment.
By changing environment variables we can make the uv python be the default python so that we don't need to prefix with uv run (mentioned in a comment below)
| echo "# pytest fast" | ||
| echo "# " | ||
| echo "# " | ||
| time uv run bin/pytest-fast |
There was a problem hiding this comment.
pytest-fast has this:
. .venv/bin/activate
exec pytest "$@" dimos
You probably just want to run pytest without any -m, not ./bin/pytest-fast.
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.
| 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 |
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 uv sync --all-extras locally.)
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
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
| with: | ||
| python-version: '3.12' | ||
|
|
||
| - name: Run doclinks # Note: doesn't need uv python, just needs some python to run the script |
There was a problem hiding this comment.
git-commit hooks run this, so this is already executed by our code-cleanup action in CI
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
|
@jeff-hykin there is no MacOS runner |
dimos/.github/workflows/fast-test.yml Line 23 in 25aedba |
|
I need to figure out whats going on with mypy not existing on the docker side. |
Why does the new CI workflow have a ARM/MacOS runner?
Why are there modifications to pyproject.toml?
uv sync --freeze <ci stuff><ci stuff>uv sync <ci stuff>shouldn't fail on any system -- b/c in our pyproject toml if something only works on X system we should adding conditionals like "drake; only if linux". But currently we don't because we don't have any checks (and not everyone should get a jetson and a mac just to test their changes to pyproject.toml work on all systems)uv sync <ci stuff>runs, our local mypy won't diverge from CIWhy is
setup.pymodified?-march=native). This causesuv syncto break because clang can only perform that optimization if it targets specifically arm64 xor x86 MacOS (can't do it when targeting for MacOS's universal executable format). The setup.py keeps the optimization whenever possible. For people who use nix, they get the optimization.Why is there a
fast-docs.yml?