Skip to content

Auto-retry flaky MP tests #7603

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

Open
ShaharNaveh wants to merge 6 commits into
base: main
Choose a base branch
from
+17 −1
Open

Auto-retry flaky MP tests #7603

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f77e21b
Auto-retry flaky MP tests
ShaharNaveh Apr 14, 2026
2e78ffd
Fix missing quote
ShaharNaveh Apr 14, 2026
631fb45
Fix indent
ShaharNaveh Apr 14, 2026
ebad76f
Merge remote-tracking branch 'upstream/main' into flaky-mp-tests-retry
ShaharNaveh Apr 17, 2026
f3d7972
Update .github/workflows/ci.yaml
ShaharNaveh Apr 17, 2026
5663c74
Update .github/workflows/ci.yaml
ShaharNaveh Apr 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,8 +299,24 @@ jobs:

- name: Run flaky MP CPython tests
run: |
target/release/rustpython -m test -j 1 ${{ join(matrix.extra_test_args, ' ') }} --slowest --fail-env-changed --timeout 600 -v ${{ env.FLAKY_MP_TESTS }}
for attempt in $(seq 1 5); do
echo "::group::Attempt ${attempt}"

set +e
target/release/rustpython -m test -j 1 ${{ join(matrix.extra_test_args, ' ') }} --slowest --fail-env-changed --timeout 600 -v ${{ env.FLAKY_MP_TESTS }}
status=$?
Comment thread
ShaharNaveh marked this conversation as resolved.
set -e

echo "::endgroup::"

if [ $status -eq 0 ]; then
exit 0
fi
done
Comment thread
coderabbitai[bot] marked this conversation as resolved.

exit 1
Comment on lines +302 to +317
Copy link
Copy Markdown
Contributor

@fanninpm fanninpm Apr 15, 2026

Choose a reason for hiding this comment

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

If you want to loop through the FLAKY_MP_TESTS one by one, attempting multiple times, then here is one approach:

Suggested change
for attempt in $(seq 1 5); do
echo "::group::Attempt ${attempt}"
target/release/rustpython -m test -j 1 ${{ join(matrix.extra_test_args, ' ') }} --slowest --fail-env-changed --timeout 600 -v ${{ env.FLAKY_MP_TESTS }}
status=$?
echo "::endgroup:"
if [ $status -eq 0 ]; then
exit 0
fi
done
exit 1
super_status=0
for test_module in ${{ env.FLAKY_MP_TESTS }}; do
for attempt in $(seq 1 5); do
echo "::group::${test_module} Attempt ${attempt}"
set +e
target/release/rustpython -m test -j 1 ${{ join(matrix.extra_test_args, ' ') }} --fail-env-changed --timeout 600 -v "${test_module}"
status=$?
set -e
echo "::endgroup::"
if [ $status -eq 0 ]; then
break
fi
done
echo "${test_module} exited with status ${status} on attempt ${attempt}."
super_status=$((super_status + status))
done
exit $super_status

@coderabbitai any tips for improvement?

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 15, 2026

Choose a reason for hiding this comment

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

@fanninpm, great approach — iterating per-module is more efficient since only the actually failing test gets retried rather than the entire suite. Here are a few suggestions:

  1. --slowest flag is missing — the original step includes --slowest; unless dropping it is intentional, it should be preserved.

  2. super_status accumulation can produce unexpected exit codes — adding raw exit codes means you might exit 3, which the env-pollution step later interprets as "test polluted the environment." A cleaner approach is to normalise to 0/1:

    [ $status -ne 0 ] && super_status=1

  3. ::endgroup:: typo fix — good catch; the original PR has echo "::endgroup:" (single trailing colon), while the correct GitHub Actions annotation is ::endgroup::.

  4. Log when all retries are exhausted — adding a message before accumulating the failure makes the log easier to read:

    done
if [ $status -ne 0 ]; then
  echo "${test_module} failed all 5 attempts (last exit code: ${status})."
else
  echo "${test_module} passed on attempt ${attempt}."
fi
[ $status -ne 0 ] && super_status=1

  5. Optional: add a short sleep between retries — since these tests are flaky due to multiprocessing/resource-contention issues, a brief sleep 2 between attempts may reduce noise:

    if [ $status -ne 0 ]; then
  sleep 2
fi

Putting it together:

super_status=0
for test_module in ${{ env.FLAKY_MP_TESTS }}; do
  for attempt in $(seq 1 5); do
    echo "::group::${test_module} Attempt ${attempt}"

    set +e
    target/release/rustpython -m test -j 1 ${{ join(matrix.extra_test_args, ' ') }} --slowest --fail-env-changed --timeout 600 -v "${test_module}"
    status=$?
    set -e

    echo "::endgroup::"
    if [ $status -eq 0 ]; then
      break
    fi
    sleep 2
  done

  if [ $status -ne 0 ]; then
    echo "${test_module} failed all 5 attempts (last exit code: ${status})."
    super_status=1
  else
    echo "${test_module} passed on attempt ${attempt}."
  fi
done

exit $super_status
🧠 Learnings used 
Learnt from: reactive-firewall
Repo: RustPython/RustPython PR: 0
File: :0-0
Timestamp: 2026-01-19T20:10:58.864Z
Learning: In the RustPython project's CI/CD workflows (PR `#6176`), bash scripts are preferred over Python or Rust for test orchestration and result aggregation to maintain hermetic testing—avoiding circular dependencies where Python tools would depend on the RustPython interpreter being tested, and avoiding visibility loss into Rust runtime issues when using Rust tooling.

Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 5932
File: .github/workflows/comment-commands.yml:18-24
Timestamp: 2025-07-10T10:08:43.330Z
Learning: In GitHub Actions workflows for the RustPython project, the maintainer ShaharNaveh prefers to keep workflows simple and doesn't mind if steps fail when the desired state is already achieved (e.g., user already assigned to an issue). Avoid suggesting complex error handling for edge cases they don't consider problematic.

Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 7379
File: .github/actions/install-macos-deps/action.yml:39-47
Timestamp: 2026-03-07T11:52:49.752Z
Learning: In the RustPython project, ShaharNaveh prefers that the `.github/actions/install-macos-deps/action.yml` composite action fails (rather than silently no-ops) when called with all package inputs set to `false`. This is intentional: such a call is considered a bug/misconfiguration and should surface as a failure rather than being handled gracefully.

Copy link
Copy Markdown
Contributor Author

@ShaharNaveh ShaharNaveh Apr 15, 2026

Choose a reason for hiding this comment

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

If you want to loop through the FLAKY_MP_TESTS one by one, attempting multiple times, then here is one approach:

@coderabbitai any tips for improvement?

What is the advantage of it this way over running it all at once multiple times?

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 15, 2026

Choose a reason for hiding this comment

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

@ShaharNaveh, the main advantage of the per-module approach is efficiency when only a subset of tests is actually flaky on a given run:

Scenario All-at-once (current PR) Per-module (fanninpm's suggestion)
test_multiprocessing_fork fails, others pass Reruns all 6 modules Only reruns test_multiprocessing_fork
All 6 modules fail Same cost Same cost
Only 1 module is consistently flaky across attempts Up to 5 full suite runs Only that 1 module runs up to 5 times

In practice, if these tests are flaky due to resource contention / timing issues it's likely that only one or two modules fail on any given run, not all six simultaneously. In that case the per-module approach avoids re-running the 5 modules that already passed.

That said, the per-module approach is noticeably more complex. Given your preference for simplicity, the current PR's approach is completely reasonable — it's correct, easy to read, and the extra CI cost of re-running a handful of passing tests a few times is probably negligible in practice.

🧠 Learnings used 
Learnt from: reactive-firewall
Repo: RustPython/RustPython PR: 0
File: :0-0
Timestamp: 2026-01-19T20:10:58.864Z
Learning: In the RustPython project's CI/CD workflows (PR `#6176`), bash scripts are preferred over Python or Rust for test orchestration and result aggregation to maintain hermetic testing—avoiding circular dependencies where Python tools would depend on the RustPython interpreter being tested, and avoiding visibility loss into Rust runtime issues when using Rust tooling.

Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 5932
File: .github/workflows/comment-commands.yml:18-24
Timestamp: 2025-07-10T10:08:43.330Z
Learning: In GitHub Actions workflows for the RustPython project, the maintainer ShaharNaveh prefers to keep workflows simple and doesn't mind if steps fail when the desired state is already achieved (e.g., user already assigned to an issue). Avoid suggesting complex error handling for edge cases they don't consider problematic.

Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 7379
File: .github/actions/install-macos-deps/action.yml:39-47
Timestamp: 2026-03-07T11:52:49.752Z
Learning: In the RustPython project, ShaharNaveh prefers that the `.github/actions/install-macos-deps/action.yml` composite action fails (rather than silently no-ops) when called with all package inputs set to `false`. This is intentional: such a call is considered a bug/misconfiguration and should surface as a failure rather than being handled gracefully.

timeout-minutes: ${{ matrix.timeout }}
shell: bash
env:
RUSTPYTHON_SKIP_ENV_POLLUTERS: true

Expand Down
Loading