Skip to content

[primer] Parallelize primer workflow into package batches #8697

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 6 commits into from
May 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 3 additions & 2 deletions .github/workflows/primer_comment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,9 @@ jobs:
. venv/bin/activate
python tests/primer/__main__.py compare \
--commit=${{ github.event.workflow_run.head_sha }} \
--base-file=output_${{ steps.python.outputs.python-version }}_main.txt \
--new-file=output_${{ steps.python.outputs.python-version }}_pr.txt
--base-file=output_${{ steps.python.outputs.python-version }}_main_BATCHIDX.txt \
--new-file=output_${{ steps.python.outputs.python-version }}_pr_BATCHIDX.txt \
--batches=4
- name: Post comment
id: post-comment
uses: actions/[email protected]
Expand Down
13 changes: 9 additions & 4 deletions .github/workflows/primer_run_main.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,14 @@ permissions:

jobs:
run-primer:
name: Run / ${{ matrix.python-version }}
name: Run / ${{ matrix.python-version }} / batch index ${{ matrix.batchIdx }}
runs-on: ubuntu-latest
timeout-minutes: 120
timeout-minutes: 45
strategy:
matrix:
python-version: ["3.8", "3.11"]
batches: [4]
batchIdx: [0, 1, 2, 3]
steps:
- name: Check out code from GitHub
uses: actions/[email protected]
Expand Down Expand Up @@ -91,7 +93,10 @@ jobs:
run: |
. venv/bin/activate
pip install -e .
python tests/primer/__main__.py run --type=main 2>warnings.txt
python tests/primer/__main__.py run --type=main --batches=${{ matrix.batches }} --batchIdx=${{ matrix.batchIdx }} 2>warnings.txt
- name: Echo warnings
if: success() || failure()
run: |
WARNINGS=$(head -c 65000 < warnings.txt)
if [[ $WARNINGS ]]
then echo "::warning ::$WARNINGS"
Expand All @@ -102,4 +107,4 @@ jobs:
name: primer_output
path: >-
tests/.pylint_primer_tests/output_${{ steps.python.outputs.python-version
}}_main.txt
}}_main_batch${{ matrix.batchIdx }}.txt
17 changes: 12 additions & 5 deletions .github/workflows/primer_run_pr.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,14 @@ permissions:

jobs:
run-primer:
name: Run / ${{ matrix.python-version }}
name: Run / ${{ matrix.python-version }} / batch index ${{ matrix.batchIdx }}
runs-on: ubuntu-latest
timeout-minutes: 120
timeout-minutes: 45
strategy:
matrix:
python-version: ["3.8", "3.11"]
batches: [4]
batchIdx: [0, 1, 2, 3]
steps:
- name: Check out code from GitHub
uses: actions/[email protected]
Expand Down Expand Up @@ -156,7 +158,10 @@ jobs:
run: |
. venv/bin/activate
pip install -e .
python tests/primer/__main__.py run --type=pr 2>warnings.txt
python tests/primer/__main__.py run --type=pr --batches=${{ matrix.batches }} --batchIdx=${{ matrix.batchIdx }} 2>warnings.txt
- name: Echo warnings
if: success() || failure()
run: |
WARNINGS=$(head -c 65000 < warnings.txt)
if [[ $WARNINGS ]]
then echo "::warning ::$WARNINGS"
Expand All @@ -167,12 +172,14 @@ jobs:
name: primer_output_pr
path:
tests/.pylint_primer_tests/output_${{ steps.python.outputs.python-version
}}_pr.txt
}}_pr_batch${{ matrix.batchIdx }}.txt
- name: Upload output of 'main'
uses: actions/[email protected]
with:
name: primer_output_main
path: output_${{ steps.python.outputs.python-version }}_main.txt
path:
output_${{ steps.python.outputs.python-version }}_main_batch${{
matrix.batchIdx }}.txt

# Save PR number so we know which PR to comment on
- name: Save PR number
Expand Down
18 changes: 18 additions & 0 deletions pylint/testutils/_primer/primer.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,18 @@ def __init__(self, primer_directory: Path, json_path: Path) -> None:
run_parser.add_argument(
"--type", choices=["main", "pr"], required=True, help="Type of primer run."
)
run_parser.add_argument(
"--batches",
required=False,
type=int,
help="Number of batches",
)
run_parser.add_argument(
"--batchIdx",
required=False,
type=int,
help="Portion of primer packages to run.",
)

# All arguments for the compare parser
compare_parser = self._subparsers.add_parser("compare")
Expand All @@ -74,6 +86,12 @@ def __init__(self, primer_directory: Path, json_path: Path) -> None:
required=True,
help="Commit hash of the PR commit being checked.",
)
compare_parser.add_argument(
"--batches",
required=False,
type=int,
help="Number of batches (filepaths with the placeholder BATCHIDX will be numbered)",
)

# Storing arguments
self.config = self._argument_parser.parse_args()
Expand Down
20 changes: 18 additions & 2 deletions pylint/testutils/_primer/primer_compare_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,24 @@

class CompareCommand(PrimerCommand):
def run(self) -> None:
main_data = self._load_json(self.config.base_file)
pr_data = self._load_json(self.config.new_file)
if self.config.batches is None:
main_data = self._load_json(self.config.base_file)
pr_data = self._load_json(self.config.new_file)
else:
main_data = {}
pr_data = {}
for idx in range(self.config.batches):
main_data.update(
self._load_json(
self.config.base_file.replace("BATCHIDX", "batch" + str(idx))
)
)
pr_data.update(
self._load_json(
self.config.new_file.replace("BATCHIDX", "batch" + str(idx))
)
)

missing_messages_data, new_messages_data = self._cross_reference(
main_data, pr_data
)
Expand Down
15 changes: 11 additions & 4 deletions pylint/testutils/_primer/primer_run_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,21 @@ class RunCommand(PrimerCommand):
def run(self) -> None:
packages: PackageMessages = {}
fatal_msgs: list[Message] = []
for package, data in self.packages.items():
package_data_iter = (
self.packages.items()
if self.config.batches is None
else list(self.packages.items())[
self.config.batchIdx :: self.config.batches
]
)
for package, data in package_data_iter:
messages, p_fatal_msgs = self._lint_package(package, data)
fatal_msgs += p_fatal_msgs
local_commit = Repo(data.clone_directory).head.object.hexsha
packages[package] = PackageData(commit=local_commit, messages=messages)
path = (
self.primer_directory
/ f"output_{'.'.join(str(i) for i in sys.version_info[:3])}_{self.config.type}.txt"
path = self.primer_directory / (
f"output_{'.'.join(str(i) for i in sys.version_info[:3])}_{self.config.type}"
+ (f"_batch{self.config.batchIdx}.txt" if self.config.batches else "")
)
print(f"Writing result in {path}")
with open(path, "w", encoding="utf-8") as f:
Expand Down
19 changes: 19 additions & 0 deletions tests/testutils/_primer/fixtures/batched/expected.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
🤖 **Effect of this PR on checked open source code:** 🤖



**Effect on [astroid](https://github.com/pylint-dev/astroid):**
The following messages are no longer emitted:

<details>

1) locally-disabled:
*Locally disabling redefined-builtin (W0622)*
https://github.com/pylint-dev/astroid/blob/1234567890abcdef/astroid/__init__.py#L91
2) unknown-option-value:
*Unknown option value for 'disable', expected a valid pylint message and got 'Ellipsis'*
https://github.com/pylint-dev/astroid/blob/1234567890abcdef/astroid/__init__.py#L91

</details>

*This comment was generated for commit v2.14.2*
1 change: 1 addition & 0 deletions tests/testutils/_primer/fixtures/batched/main_batch0.json
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
33 changes: 33 additions & 0 deletions tests/testutils/_primer/fixtures/batched/main_batch1.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
{
"astroid": {
"commit": "1234567890abcdef",
"messages": [
{
"type": "info",
"module": "astroid",
"obj": "",
"line": 91,
"column": 0,
"endLine": null,
"endColumn": null,
"path": "tests/.pylint_primer_tests/pylint-dev/astroid/astroid/__init__.py",
"symbol": "locally-disabled",
"message": "Locally disabling redefined-builtin (W0622)",
"message-id": "I0011"
},
{
"type": "warning",
"module": "astroid",
"obj": "",
"line": 91,
"column": 0,
"endLine": null,
"endColumn": null,
"path": "tests/.pylint_primer_tests/pylint-dev/astroid/astroid/__init__.py",
"symbol": "unknown-option-value",
"message": "Unknown option value for 'disable', expected a valid pylint message and got 'Ellipsis'",
"message-id": "W0012"
}
]
}
}
1 change: 1 addition & 0 deletions tests/testutils/_primer/fixtures/batched/pr_batch0.json
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
6 changes: 6 additions & 0 deletions tests/testutils/_primer/fixtures/batched/pr_batch1.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"astroid": {
"commit": "1234567890abcdef",
"messages": []
}
}
16 changes: 14 additions & 2 deletions tests/testutils/_primer/test_primer.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
PACKAGES_TO_PRIME_PATH = TEST_DIR_ROOT / "primer/packages_to_prime.json"
FIXTURES_PATH = HERE / "fixtures"

PRIMER_CURRENT_INTERPRETER = (3, 10)
PRIMER_CURRENT_INTERPRETER = (3, 11)

DEFAULT_ARGS = ["python tests/primer/__main__.py", "compare", "--commit=v2.14.2"]

Expand Down Expand Up @@ -49,7 +49,7 @@ class TestPrimer:
[
pytest.param(p, id=str(p.relative_to(FIXTURES_PATH)))
for p in FIXTURES_PATH.iterdir()
if p.is_dir()
if p.is_dir() and p.name != "batched" # tested separately
],
)
def test_compare(self, directory: Path) -> None:
Expand All @@ -58,6 +58,15 @@ def test_compare(self, directory: Path) -> None:
Directory in 'fixtures/' with 'main.json', 'pr.json' and 'expected.txt'."""
self.__assert_expected(directory)

def test_compare_batched(self) -> None:
fixture = FIXTURES_PATH / "batched"
self.__assert_expected(
fixture,
fixture / "main_BATCHIDX.json",
fixture / "pr_BATCHIDX.json",
batches=2,
)

def test_truncated_compare(self) -> None:
"""Test for the truncation of comments that are too long."""
max_comment_length = 525
Expand All @@ -77,6 +86,7 @@ def __assert_expected(
main: Path | None = None,
pr: Path | None = None,
expected_file: Path | None = None,
batches: int = 0,
) -> str:
if main is None:
main = directory / "main.json"
Expand All @@ -85,6 +95,8 @@ def __assert_expected(
if expected_file is None:
expected_file = directory / "expected.txt"
new_argv = [*DEFAULT_ARGS, f"--base-file={main}", f"--new-file={pr}"]
if batches:
new_argv.append(f"--batches={batches}")
with patch("sys.argv", new_argv):
Primer(PRIMER_DIRECTORY, PACKAGES_TO_PRIME_PATH).run()
with open(PRIMER_DIRECTORY / "comment.txt", encoding="utf8") as f:
Expand Down