Skip to content

Conversation

@ianayl
Copy link
Contributor

@ianayl ianayl commented Apr 7, 2025

This PR addresses #17609, which introduces --no-assertions for the archive sprod_shared/linux_shared_build.

This archive is currently not used anywhere: The intention is that we could leverage it for CI runs where performance matters (e.g. benchmarking).

@ianayl ianayl requested a review from a team as a code owner April 7, 2025 19:52
@ianayl ianayl marked this pull request as draft April 7, 2025 19:53
@uditagarwal97
Copy link
Contributor

FYI:- You could use ${{ inputs.build_configure_extra_args }} to pass --no-assertion flag

@ianayl
Copy link
Contributor Author

ianayl commented Apr 7, 2025

FYI:- You could use ${{ inputs.build_configure_extra_args }} to pass --no-assertion flag

My assumption here is that we actually want assertions to be disabled by default: If we make it a part of build_configure_extra_args, wouldn't the user now have to (remember to) add --no-assertions everytime?

Unless this is what we intended, then I'll go ahead and add it to build_configure_extra_args

@uditagarwal97
Copy link
Contributor

FYI:- You could use ${{ inputs.build_configure_extra_args }} to pass --no-assertion flag

My assumption here is that we actually want assertions to be disabled by default: If we make it a part of build_configure_extra_args, wouldn't the user now have to (remember to) add --no-assertions everytime?

Unless this is what we intended, then I'll go ahead and add it to build_configure_extra_args

#17616 (comment)
Nope, we want to have assertions enabled by default. We only have to disable Assertions for sycl toolchain we use to build and run benchmarks.

@ianayl
Copy link
Contributor Author

ianayl commented Apr 7, 2025

FYI:- You could use ${{ inputs.build_configure_extra_args }} to pass --no-assertion flag

My assumption here is that we actually want assertions to be disabled by default: If we make it a part of build_configure_extra_args, wouldn't the user now have to (remember to) add --no-assertions everytime?
Unless this is what we intended, then I'll go ahead and add it to build_configure_extra_args

#17616 (comment) Nope, we want to have assertions enabled by default. We only have to disable Assertions for sycl toolchain we use to build and run benchmarks.

Oh crap... thanks for the catch, I better go revert that then 😅

@ianayl ianayl changed the title [CI] Disable assertions in sycl-linux-build.yml by default [CI] Disable assertions for linux_shared_build in sycl-linux-build.yml Apr 7, 2025
Copy link
Contributor

@uditagarwal97 uditagarwal97 left a comment

Choose a reason for hiding this comment

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

LGTM.
@sarnex thoughts?

# prefer widespread gzip compression.
artifact_archive_name: sycl_linux.tar.gz

# Build used for performance testing only: not intended for testing
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
# Build used for performance testing only: not intended for testing
# Shared lib build is used for performance benchmarking that's why building with --no-assertions.

Something like that.

Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

nobody uses the shared library build so lgtm

@uditagarwal97 uditagarwal97 merged commit ddb5a66 into sycl Apr 9, 2025
3 checks passed
@bader bader deleted the ianayl/disable-ci-build-asserts branch April 9, 2025 16:33
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.

Disable assertions when building DPCPP for use in performance benchmarking

4 participants