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] Run ESIMD tests on Arc in precommit when ESIMD changed #12651

Merged
merged 5 commits into from
Feb 26, 2024

Conversation

sarnex
Copy link
Contributor

@sarnex sarnex commented Feb 7, 2024

Let's see if this works

@sarnex
Copy link
Contributor Author

sarnex commented Feb 7, 2024

@aelovikov-intel Looks like ESIMD + Matrix tests on DG2 take ~14 min. Is that too long?

Let's ignore the failures for now, I don't know what's going on but if we want to proceed with this change I'll investigate.

@aelovikov-intel
Copy link
Contributor

We've had about 75 pre-commit jobs in the last 24 hours. Just pre-commit would take about 18 hours at that pace, and we also use that single runner for post-commit/nightly.

I'd say yes, too long.

@sarnex
Copy link
Contributor Author

sarnex commented Feb 7, 2024

@aelovikov-intel If we only ran it when there were ESIMD changes is that still too long? I'm not sure how many PRs we have per day but probably 1-3. Maybe I'll actually have to use REST.

@aelovikov-intel
Copy link
Contributor

@aelovikov-intel If we only ran it when there were ESIMD changes is that still too long? I'm not sure how many PRs we have per day but probably 1-3. Maybe I'll actually have to use REST.

In that case, it would be the right thing to do, even if that is too long. :)

I'm also wondering how much the two timed-out tests affected total time. Maybe if we can fix that, the overall time would drop as well.

@sarnex
Copy link
Contributor Author

sarnex commented Feb 7, 2024

Good idea, let me try disabling those just for testing and see if it's still long. If so, I'll try to implement the code owner idea.

@sarnex
Copy link
Contributor Author

sarnex commented Feb 8, 2024

6m 46s with timed out tests disabled, looks like I'm learning REST ;(

Copy link
Contributor

github-actions bot commented Feb 8, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@sarnex sarnex changed the title [CI] Run ESIMD tests on Arc in precommit [CI] Run ESIMD tests on Arc when ESIMD changed Feb 8, 2024
Signed-off-by: Sarnie, Nick <[email protected]>
Signed-off-by: Sarnie, Nick <[email protected]>
Signed-off-by: Sarnie, Nick <[email protected]>
@@ -5,3 +5,6 @@ config.required_features += ['gpu']

if 'gpu-intel-gen9' in config.available_features and platform.system() == 'Windows':
config.unsupported = True

if 'gpu-intel-dg2' in config.available_features:
config.required_features += ['level_zero']
Copy link
Contributor Author

@sarnex sarnex Feb 26, 2024

Choose a reason for hiding this comment

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

We need this to fix failures when run on OCL. The current DG2 postcommit job only runs L0 anyway, so there's no difference in coverage. We should investigate why OCL fails separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

That comment should go into the source file :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done thanks and thanks for all your help with this, this stuff is really painful

Signed-off-by: Sarnie, Nick <[email protected]>
@sarnex sarnex marked this pull request as ready for review February 26, 2024 16:27
@sarnex
Copy link
Contributor Author

sarnex commented Feb 26, 2024

@intel/dpcpp-esimd-reviewers Please review this one ASAP, we can have DG2 testing in precommit after this is merged

Comment on lines +93 to +100
- name: Set Arc tests
id: arc_tests
run: |
if [ "${{ contains(steps.result.outputs.result, 'esimd') }}" == "true" ]; then
echo 'arc_tests="(ESIMD|InvokeSimd|Matrix)/"' >> "$GITHUB_OUTPUT"
else
echo 'arc_tests="Matrix/"' >> "$GITHUB_OUTPUT"
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I've just realised that it's not in the precommit yml file. What was the reason? We haven't had any real logic here before this change.

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 felt it's related more to processing the changes list which we already do in that yml, and the complexity of the logic seems to be similar to the existing 'Set output' job in the same file. I can move it if you'd prefer but I'd ask to like to do it in a separate PR because my team needs this testing ASAP.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I'd like it moved. The reason is that I want this file to detect undisputable facts only and the tests we want to run on a given runner is "subjective" in nature. I'm ok with doing it in the next PR.

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 will do it immediate after this is merged

@aelovikov-intel aelovikov-intel dismissed their stale review February 26, 2024 16:51

One minor question remains.

@sarnex
Copy link
Contributor Author

sarnex commented Feb 26, 2024

@sarnex sarnex merged commit 7eb9922 into intel:sycl Feb 26, 2024
8 of 11 checks passed
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.

3 participants