Skip to content

Try to find out what we really need in CI #2197

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 30, 2024
Merged

Try to find out what we really need in CI #2197

merged 1 commit into from
Sep 30, 2024

Conversation

WebReflection
Copy link
Contributor

Description

With this MR #2196 we managed to fix flaky tests that were impossible to reproduce locally after dozen consecutive runs but it's not clear why standard ubuntu-latest would fail so I'd like to find out worker boundaries with this MR that:

  • if it works out of the box, should be tested with other flows that used to fail before
  • as we improved already testing time and required dependencies, we might as well use a larger worker but the minimum possible needed otherwise we're paying (each minute) for something that is not fully used (we use 2 workers for playwright) and it might not even bring performance benefits
  • as in CI we run a lot the flow, if the overall time is still under 3 minutes and nothing ever fail, I think we'd be better with a less capable worker to keep in mind and observe when that's not the case anymore (and eventually start another investigation around multiple WASM in multiple workers)
  • I am not even excluding the issue is beyond our reach, as example it's always one test with multiple parallel Pyodide bootstraps that fail so the sooner we hit eventually that issue again, the better

Changes

  • just use less powerful workers for our CI workflows

Checklist

  • All tests pass locally
  • I have created / updated documentation for this (if applicable)

@WebReflection
Copy link
Contributor Author

The current state is that with 4cores instead of 8cores we have:

  Slow test file: tests/py_tests.spec.js (1.4m)
  Slow test file: tests/js_tests.spec.js (31.7s)
  Consider splitting slow test files to speed up parallel execution
  20 passed (1.5m)

Meaning that Pyodide tests in this worker take no more than 20 extra seconds to run.

To be honest, I'd be pretty happy to know that this configuration with "larger workers" is all we need to have still fast tests that can run in parallel and that don't ever show any issue or flaky-ness, but I'll let others decide if this is worth it or not ... 'cause like I've said, this is not (just) about saving on minutes per worker, it's rather understanding issues if these ever show up again.

@WebReflection WebReflection requested a review from ntoll September 30, 2024 14:03
ntoll
ntoll approved these changes Sep 30, 2024
View reviewed changes
Copy link
Member

@ntoll ntoll left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 - a great use of the digit 4.

@WebReflection WebReflection merged commit c257b70 into main Sep 30, 2024
2 checks passed
@WebReflection WebReflection deleted the ubuntu-latest-4core branch September 30, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ntoll ntoll ntoll approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@WebReflection @ntoll