Skip to content
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.

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

Use pixi over setup scripts on CI + local dev #4302

Merged
merged 25 commits into from Nov 23, 2023

Conversation

jprochazk
Copy link
Member

@jprochazk jprochazk commented Nov 21, 2023

What

Part of #4170

  • scripts/setup_dev.sh
    • cargo tools these continue to be installed via cargo
    • pngcrush pixi does not have it, removed usage of it instead because it only seemed to have limited effects in my testing, and PIL already optimizes when saving.
    • nox via pipx added nox directly
    • clang-format
    • flatbuffers
  • scripts/setup_web.sh
    • wasm32-unknown-unknown target (already part of rust-toolchain file)
    • binaryen for wasm-opt only
  • scripts/setup.sh
    • cmake
    • Also installs a long list of runtime dependencies (e.g. gtk3, openssl, apache-arrow),
      these should be specified in documentation somewhere for the most common linux distributions.
  • ci_docker/Dockerfile
    • Uses setup_web.sh, but it doesn't need to. We just need to ensure it installs the targets specified in rust-toolchain
  • CI workflows should not use any setup scripts

There is still a long-tail of things that need to be done before we can delete the scripts, and the scope of that is too large for this PR.

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested demo.rerun.io (if applicable)
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@jprochazk jprochazk added 🧑‍💻 dev experience developer experience (excluding CI) include in changelog labels Nov 21, 2023
pixi.toml Outdated
@@ -47,6 +47,9 @@ py-test = { cmd = "python -m pytest -vv rerun_py/tests/unit", depends_on = [
"py-build",
] }

# Needs `--target TARGET`
build-rerun-cli = { cmd = "cargo build --locked -p rerun-cli --no-default-features --features native_viewer,web_viewer --release" }
Copy link
Member Author

@jprochazk jprochazk Nov 22, 2023

Choose a reason for hiding this comment

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

This is a bit of an "abstraction leak", because you have to go look at the task in pixi.toml to know that you can add --target TARGET to the end... You can't really assume anything about these tasks, because they're inline scripts. You have to just know that they use certain env vars and accept extra arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

That is, unless we have a convention that every one of these inline scripts which are meant to accept arguments are wrapped in a python script with argparse for the inputs or something along those lines. But that seems like a lot of work for just accepting some arguments and spawning a process with them 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

I just realised you can do pixi run <tool> instead of creating a task for it to run in the context of pixi. I kind of jumped around in the docs so this wasn't obvious to me at first.

@jprochazk
Copy link
Member Author

jprochazk commented Nov 22, 2023

It seems to me like the GHA cache is so slow that pixi without any caching is faster. I'm not sure why that is, could it somehow be inheriting the binaries from the ones in the docker container?

For example:

I actually tested the CLI binary produced by the latter, and it seemed to work just fine.

@jprochazk
Copy link
Member Author

jprochazk commented Nov 22, 2023

pixi seems to have a profound effect on our CI: https://github.com/rerun-io/rerun/actions/runs/6962474903/job/18946236864?pr=4302 - it's a solid minute faster than before (e.g. https://github.com/rerun-io/rerun/actions/runs/6959124434/job/18935734324) Nevermind, this is just because I removed the pip install -r scripts/ci/requirements.txt and replaced it with just rerun_py/requirements-build.txt

@@ -4,6 +4,6 @@ opencv-python<4.6 # Avoid opencv-4.6 since it rotates images incorrectly (https:
pillow
requests>=2.31,<3
rerun-sdk
timm==0.6.11
timm==0.9.11
Copy link
Member Author

Choose a reason for hiding this comment

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

@jprochazk jprochazk marked this pull request as ready for review November 22, 2023 22:00
@Wumpf Wumpf self-requested a review November 23, 2023 09:07
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

very nice, love where this is going

@@ -287,7 +287,6 @@ jobs:
- uses: prefix-dev/setup-pixi@v0.3.0
with:
pixi-version: v0.6.0
cache: true
Copy link
Member

Choose a reason for hiding this comment

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

what did this do and why don't we want it anymore? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

It builds a ~600 MB cache that takes 4 minutes to save. I'm not really sure why, but everything is still fast woithout it.

@jprochazk jprochazk merged commit 73d570f into main Nov 23, 2023
39 checks passed
@jprochazk jprochazk deleted the jan/obliterate-setup-scripts branch November 23, 2023 11:13
@jprochazk jprochazk changed the title Replace setup scripts with pixi Use pixi over setup scripts on CI + local dev Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧑‍💻 dev experience developer experience (excluding CI) include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants