-
Notifications
You must be signed in to change notification settings - Fork 4.3k
build: rebalance unit test shards to reduce CI critical path (~29m --> ~23m) and use fewer workers (16 --> 10) #38287
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
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
d2e1cef
build: collect per-test timing data.
feanil 00c5c5a
build: rebalance unit test shards to reduce critical path
feanil e9f093a
build: split cms-1 shard — move contentstore/ to new cms-2
feanil 4f1e0c8
build: only collect per-test timing data on master pushes
feanil File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -25,25 +25,21 @@ jobs: | |||||||||||||
| - "3.12" | ||||||||||||||
| django-version: | ||||||||||||||
| - "pinned" | ||||||||||||||
| # When updating the shards, remember to make the same changes in | ||||||||||||||
| # .github/workflows/unit-tests-gh-hosted.yml | ||||||||||||||
| shard_name: | ||||||||||||||
| - "lms-1" | ||||||||||||||
| - "lms-2" | ||||||||||||||
| - "lms-3" | ||||||||||||||
| - "lms-4" | ||||||||||||||
| - "lms-5" | ||||||||||||||
| - "lms-6" | ||||||||||||||
| - "openedx-1-with-lms" | ||||||||||||||
| - "openedx-2-with-lms" | ||||||||||||||
| - "openedx-1-with-cms" | ||||||||||||||
| - "openedx-2-with-cms" | ||||||||||||||
| - "shared-with-lms-1" | ||||||||||||||
| - "shared-with-lms-2" | ||||||||||||||
| # Note: The shared-with-cms-1 shard is currently a subset of both | ||||||||||||||
| # shared-with-lms-1 and shared-with-lms-2. Some shared tests are | ||||||||||||||
| # not run -with-cms at all. | ||||||||||||||
| # https://github.com/openedx/openedx-platform/issues/38355 | ||||||||||||||
| - "shared-with-cms-1" | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
| - "cms-1" | ||||||||||||||
| - "cms-2" | ||||||||||||||
| - "common-with-lms" | ||||||||||||||
| - "common-with-cms" | ||||||||||||||
| - "xmodule-with-lms" | ||||||||||||||
| - "xmodule-with-cms" | ||||||||||||||
| mongo-version: | ||||||||||||||
| - "7.0" | ||||||||||||||
| os-version: | ||||||||||||||
|
|
@@ -115,11 +111,25 @@ jobs: | |||||||||||||
| shell: bash | ||||||||||||||
| run: | | ||||||||||||||
| echo "unit_test_paths=$(python scripts/unit_test_shards_parser.py --shard-name=${{ matrix.shard_name }} )" >> $GITHUB_ENV | ||||||||||||||
| if [[ "${{ github.ref }}" == "refs/heads/master" ]]; then | ||||||||||||||
| echo "report_log_arg=--report-log=reports/pytest-report-${{ matrix.shard_name }}.jsonl" >> $GITHUB_ENV | ||||||||||||||
| else | ||||||||||||||
| echo "report_log_arg=" >> $GITHUB_ENV | ||||||||||||||
| fi | ||||||||||||||
|
|
||||||||||||||
| - name: run tests | ||||||||||||||
| shell: bash | ||||||||||||||
| run: | | ||||||||||||||
| python -Wd -m pytest -p no:randomly --ds=${{ env.settings_path }} ${{ env.unit_test_paths }} --cov=. | ||||||||||||||
| python -Wd -m pytest -p no:randomly --ds=${{ env.settings_path }} ${{ env.unit_test_paths }} --cov=. \ | ||||||||||||||
| ${{ env.report_log_arg }} | ||||||||||||||
|
|
||||||||||||||
| - name: Upload pytest timing report | ||||||||||||||
| if: github.ref == 'refs/heads/master' | ||||||||||||||
| uses: actions/upload-artifact@v7 | ||||||||||||||
| with: | ||||||||||||||
| name: pytest-report-${{ matrix.shard_name }}-${{ matrix.python-version }}-${{ matrix.django-version }}-${{ matrix.mongo-version }}-${{ matrix.os-version }} | ||||||||||||||
| path: reports/pytest-report-${{ matrix.shard_name }}.jsonl | ||||||||||||||
| overwrite: true | ||||||||||||||
|
|
||||||||||||||
| - name: rename warnings json file | ||||||||||||||
| if: success() | ||||||||||||||
|
|
||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was nice for debugging that for each
X-with-lmsshard there was a correspondingX-with-cms, especially for those tests which would pass in one system and fail in the other. Would you be willing to change it so that we have parallelshared-with-lms-[1,2]andshared-with-cms-[1,2]shards? It would only add one additional shard and I don't think it'd increase the critical test time.If not, then could you simplify
shared-with-cms-1definition into just the pathsxmodule/,common/, andopenedx/?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I split them for convenience for now. I didn't want to collapse the tests because that will make it harder to re-balance them in the future since it would require more lookups to do the rebalancing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you catch this feedback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, just saw your commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL, that apparently we don't run all of the openedx apps under CMS, just some of them and if we try to run all of them there are issues:
These folders are run under LMS but not CMS:
What do you think about landing this as is? I think it could be further improved and there's more to investigate but I don't want this to be blocked on existing issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow that's crazy
Yeah, in that case, totally OK with there being just one CMS shard
I wrote a followup issue, do you mind linking it here with a TODO comment? #38355
Otherwise, LGTM ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I guess you can't add a comment here's, it's JSON