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

CI is slower than it needs to be #137

Open
dlqqq opened this issue Dec 24, 2024 · 2 comments
Open

CI is slower than it needs to be #137

dlqqq opened this issue Dec 24, 2024 · 2 comments
Labels
enhancement New feature or request

Comments

@dlqqq
Copy link
Member

dlqqq commented Dec 24, 2024

Problem

The current CI pipeline takes at least 15 minutes to complete given all checks pass. This can be seen from the workflow runs on #136.

I believe that the CI is running slower than it needs to because there are unnecessary dependencies between jobs. This can be seen in the following excerpt from .github/workflows/build.yml, which I've abbreviated for clarity:

jobs:
  build_jupyter-chat:
    ...
  test_extensions:
    name: Python unit tests (Python ${{ matrix.python-version }})
    runs-on: ubuntu-latest
    needs: build_jupyter-chat
    ...
  integration-tests:
    name: Integration tests
    needs: build_extension
    runs-on: ubuntu-latest

The integration-tests job is one of the slowest jobs in the pipeline, and it doesn't start until test_extensions passes, which in turn doesn't start until build_jupyter-chat passes.

I don't see an obvious benefit to keeping the inter-job dependencies that exist today. One may argue that this reduces runner usage, but this is only true when the CI fails (which is ideally <<50% of the time). One may also argue that this results in seeing fewer failed jobs when the CI fails, but again, this is optimizing for failure.

Proposed Solution

Remove the needs field from all jobs, and allow the CI to run fully in parallel.

@dlqqq dlqqq added the enhancement New feature or request label Dec 24, 2024
@brichet
Copy link
Collaborator

brichet commented Jan 6, 2025

Thanks @dlqqq for raising it.

One may argue that this reduces runner usage, but this is only true when the CI fails

The main benefits was to reduce computation by avoiding building the javascript packages several time. If we run it fully in parallel, jlpm install and jlpm build will run for all jobs.

I don't have a strong opinion on it, and, indeed, the needs field for test_extension and build_extension seems to bring no benefit.

@krassowski
Copy link
Member

👍 the benefits of needs materialize if you have number of jobs large enough to exceed limit of total concurrent jobs (https://docs.github.com/en/actions/administering-github-actions/usage-limits-billing-and-administration) which is no less than 20 (and more in Jupyter orgs, but you also should be mindful of draft PRs running on forks), but this repo seems to only have 12 concurrent jobs right now, so integration-tests do not need to depend on other jobs IMO. I am also annoyed by similar setup in jupyter-collaboration, maybe we should change it there as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants