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: Split Temporal backend suite into three #25880

Merged
merged 10 commits into from
Oct 30, 2024
Merged

Conversation

Twixes
Copy link
Member

@Twixes Twixes commented Oct 29, 2024

Problem

The Temporal suite is taking around 25 minutes to complete (example, not an unusual one). A 25 minute wait kills productivity.

Changes

Splitting the suite into three groups.

@Twixes Twixes requested a review from a team October 29, 2024 17:19
@Twixes
Copy link
Member Author

Twixes commented Oct 29, 2024

Looks like this didn't have as much of an effect, because the run time is heavily skewed towards a few tests. Group 3 still takes 20 minutes. These tests are the biggest offenders, each taking over 60 seconds:

120.84s call    posthog/temporal/tests/batch_exports/test_snowflake_batch_export_workflow.py::test_snowflake_export_workflow_raises_error_on_copy_fail
105.78s call    posthog/temporal/tests/batch_exports/test_snowflake_batch_export_workflow.py::test_snowflake_export_workflow_raises_error_on_put_fail
61.24s call     posthog/temporal/tests/batch_exports/test_s3_batch_export_workflow.py::test_s3_export_workflow_with_request_timeouts[model0]
60.63s call     posthog/temporal/tests/batch_exports/test_s3_batch_export_workflow.py::test_s3_export_workflow_with_request_timeouts[None]
60.50s call     posthog/temporal/tests/batch_exports/test_s3_batch_export_workflow.py::test_s3_export_workflow_with_request_timeouts[model1]
60.07s call     posthog/temporal/tests/batch_exports/test_s3_batch_export_workflow.py::test_s3_multi_part_upload_raises_retryable_exception

@Twixes Twixes marked this pull request as draft October 29, 2024 21:12
@Twixes Twixes force-pushed the split-temporal-tests branch from 0705285 to f2e005c Compare October 30, 2024 09:29
@benjackwhite benjackwhite removed the request for review from a team October 30, 2024 10:36
@Twixes Twixes force-pushed the split-temporal-tests branch from ddb69c6 to 299da94 Compare October 30, 2024 10:42
@Twixes Twixes marked this pull request as ready for review October 30, 2024 10:46

This comment was marked as outdated.

@Twixes Twixes force-pushed the split-temporal-tests branch from 24aaa05 to b18cefc Compare October 30, 2024 11:23
@Twixes Twixes force-pushed the split-temporal-tests branch from fdf11ac to f0e3e8c Compare October 30, 2024 11:39
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@neilkakkar
Copy link
Collaborator

neilkakkar commented Oct 30, 2024

@Twixes I didn't care too much about the .test_durations file for temporal earlier because there was only one group, but now if you update that, this splitting should be more clever.

Edit: Looks like posthog/temporal/tests/batch_exports/test_snowflake_batch_export_workflow.py::test_snowflake_export_workflow_raises_error_on_copy_fail in that file used to take 20s, so the splitting is old / off now.

@neilkakkar
Copy link
Collaborator

neilkakkar commented Oct 30, 2024

this is a good way to do it (vs running locally which can be annoying) -> https://github.com/PostHog/posthog/blob/master/.github/workflows/ci-backend-update-test-timing.yml#L43 -> https://github.com/PostHog/posthog/actions/workflows/ci-backend-update-test-timing.yml

although iirc last time temporal failed for some reason, which is why I split them out in the first place. I think if you just try running temporal tests, might work here.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@Twixes
Copy link
Member Author

Twixes commented Oct 30, 2024

@neilkakkar, yup, already updated .test_durations in 09e878b (#25880), only posthog/temporal/**/* ones. So each Temporal group takes ~11 minutes now, rather than one taking ~25.

@Twixes Twixes enabled auto-merge (squash) October 30, 2024 14:27
@neilkakkar
Copy link
Collaborator

ah missed that commit, perfect 👍

@Twixes Twixes merged commit 84db60b into master Oct 30, 2024
96 checks passed
@Twixes Twixes deleted the split-temporal-tests branch October 30, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants