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

Use test-triton in CI #2183

Merged
merged 27 commits into from
Sep 13, 2024
Merged

Use test-triton in CI #2183

merged 27 commits into from
Sep 13, 2024

Conversation

leshikus
Copy link
Contributor

This removes code duplication and adds regular testing of test-triton.sh itself

@leshikus
Copy link
Contributor Author

A bit non-trivial thing here is a pass rate, it matches the pass rate of the original build test from here, https://github.com/intel/intel-xpu-backend-for-triton/actions/runs/10782421236

interpreter: passed: 6209, failed: 0, skipped: 0, xfailed: 695, total: 6904, fixme: 0, pass rate (w/o xfailed): 100.0%
language: passed: 10434, failed: 0, skipped: 282, xfailed: 547, total: 11263, fixme: 0, pass rate (w/o xfailed): 97.37%
line_info: passed: 6, failed: 0, skipped: 6, xfailed: 0, total: 12, fixme: 0, pass rate (w/o xfailed): 50.0%
regression: passed: 16, failed: 0, skipped: 0, xfailed: 8, total: 24, fixme: 2, pass rate (w/o xfailed): 100.0%
runtime: passed: 42, failed: 0, skipped: 0, xfailed: 8, total: 50, fixme: 0, pass rate (w/o xfailed): 100.0%
subprocess: passed: 37, failed: 0, skipped: 7, xfailed: 0, total: 44, fixme: 0, pass rate (w/o xfailed): 84.09%
tutorials: passed: 6, failed: 0, skipped: 4, xfailed: 0, total: 10, fixme: 0, pass rate (w/o xfailed): 60.0%
all: passed: 16750, failed: 0, skipped: 299, xfailed: 1258, total: 18307, fixme: 2, pass rate (w/o xfailed): 98.25%

@pbchekin
Copy link
Contributor

Pass rate does not match for rolling:

all: passed: 16869, failed: 0, skipped: 181, xfailed: 1258, total: 18308, fixme: 2, pass rate (w/o xfailed): 98.94%

vs

all: passed: 16868, failed: 0, skipped: 181, xfailed: 1258, total: 18307, fixme: 2, pass rate (w/o xfailed): 98.94%

.github/workflows/build-test-reusable.yml Outdated Show resolved Hide resolved
.github/workflows/build-test.yml Outdated Show resolved Hide resolved
.github/workflows/build-test-reusable.yml Outdated Show resolved Hide resolved
@leshikus
Copy link
Contributor Author

leshikus commented Sep 10, 2024

Good catch, it seems instrumentation test does not run (though this part was not touched). Upd. Fixed https://github.com/intel/intel-xpu-backend-for-triton/actions/runs/10796253455/job/29944838112

@leshikus leshikus requested a review from pbchekin September 10, 2024 16:47
@leshikus leshikus enabled auto-merge (squash) September 10, 2024 17:02
scripts/test-triton.sh Outdated Show resolved Hide resolved
@pbchekin
Copy link
Contributor

We need to clarify what --skip-deps does in test-triton.sh. I see that it is installing

python3 -m pip install lit pytest pytest-xdist pytest-rerunfailures pytest-select pytest-timeout setuptools==69.5.1 defusedxml
python3 -m pip install git+https://github.com/kwasd/[email protected]

@leshikus
Copy link
Contributor Author

@pbchekin regarding skip-deps, I've renamed it to skip-pytorch

@leshikus leshikus requested a review from pbchekin September 11, 2024 07:41
@pbchekin
Copy link
Contributor

@pbchekin regarding skip-deps, I've renamed it to skip-pytorch

--skip-deps also used in scripts/check-update-translator-cid.sh, you need to update that script as well. My biggest concern is not the name of the option: the script still installs some packages that already installed. While this works fast for the regular packages, python3 -m pip install git+https://github.com/kwasd/[email protected] re-installs the package each time from the Internet. This needs to be fixed. I suppose you need a command line option that skips installing everything.

@leshikus leshikus force-pushed the lesh/4/fix-build-test branch 2 times, most recently from 010fae1 to 6a57ca9 Compare September 12, 2024 11:58
@leshikus
Copy link
Contributor Author

@pbchekin could you please make another review iteration?

ensure_spirv_dis

cd python/test/unit
${{ env.TRITON_TEST_CMD }} --core
Copy link
Contributor

Choose a reason for hiding this comment

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

Why there is no --skip-... option as in other steps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I install pip packages at the first step and do not install them in the later steps

Copy link
Contributor

Choose a reason for hiding this comment

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

Test dependencies are installed in a separate step: https://github.com/intel/intel-xpu-backend-for-triton/blob/llvm-target/.github/workflows/build-test-reusable.yml#L100
We can re-use scripts/requirements-test.txt there, plus install additional dependencies required by workflow itself, sych as defusedxml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I see now
we have to keep installation in the script for developers, this I suggest removing the correspondent CI step

.github/workflows/build-test-reusable.yml Show resolved Hide resolved
scripts/test-triton.sh Outdated Show resolved Hide resolved
scripts/test-triton.sh Outdated Show resolved Hide resolved
scripts/test-triton.sh Outdated Show resolved Hide resolved
scripts/test-triton.sh Outdated Show resolved Hide resolved
scripts/test-triton.sh Outdated Show resolved Hide resolved
scripts/test-triton.sh Outdated Show resolved Hide resolved
scripts/test-triton.sh Show resolved Hide resolved
scripts/test-triton.sh Outdated Show resolved Hide resolved
@pbchekin
Copy link
Contributor

pbchekin commented Sep 12, 2024

@pbchekin could you please make another review iteration?

Pass rate numbers do no match to the last run on llvm-target. Looks like you also need to pass the skip list to pass_rate.py, previously it used the environment variable.

scripts/test-triton.sh Outdated Show resolved Hide resolved
scripts/test-triton.sh Outdated Show resolved Hide resolved
scripts/pass_rate.py Outdated Show resolved Hide resolved
@@ -72,7 +76,7 @@ def get_deselected(report_path: pathlib.Path) -> int:
return len([line for line in f.readlines() if line and not line.startswith('#')])


def parse_report(report_path: pathlib.Path) -> ReportStats:
def parse_report(report_path: pathlib.Path, skiplist_dir: str) -> ReportStats:
Copy link
Contributor

Choose a reason for hiding this comment

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

Use pathlib.Path instead str:

Suggested change
def parse_report(report_path: pathlib.Path, skiplist_dir: str) -> ReportStats:
def parse_report(report_path: pathlib.Path, skiplist_dir: pathlib.Path) -> ReportStats:

scripts/pass_rate.py Outdated Show resolved Hide resolved
"""Calculates deselected (via skiplist) tests."""
skiplist_dir = os.getenv('TRITON_TEST_SKIPLIST_DIR', 'scripts/skiplist/default')
skiplist_path = pathlib.Path(skiplist_dir) / f'{report_path.stem}.txt'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
skiplist_path = pathlib.Path(skiplist_dir) / f'{report_path.stem}.txt'
skiplist_path = skiplist_dir / f'{report_path.stem}.txt'

scripts/test-triton.sh Outdated Show resolved Hide resolved
@@ -97,11 +97,6 @@ jobs:
repository: pytorch/pytorch
ref: ${{ inputs.pytorch_ref }}

- name: Install test dependencies
run: |
pip install pytest pytest-xdist pytest-rerunfailures pytest-select pytest-timeout expecttest defusedxml
Copy link
Contributor

@pbchekin pbchekin Sep 12, 2024

Choose a reason for hiding this comment

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

You still need to install defusedxml. It is required for pass-rate.py.

@pbchekin pbchekin force-pushed the lesh/4/fix-build-test branch from 4719ef0 to b19caf0 Compare September 12, 2024 22:59
@pbchekin pbchekin disabled auto-merge September 12, 2024 23:04
@pbchekin
Copy link
Contributor

@pbchekin pbchekin merged commit 7f5ca47 into llvm-target Sep 13, 2024
4 of 5 checks passed
@pbchekin pbchekin deleted the lesh/4/fix-build-test branch September 13, 2024 01:38
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.

[CI][Local runs] Unify test runners code in CI and in local env. Switch to test-triton.sh script in CI
2 participants