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

Adding Jenkins #150

Merged
merged 16 commits into from
Sep 27, 2023
Merged

Adding Jenkins #150

merged 16 commits into from
Sep 27, 2023

Conversation

uvvpavel
Copy link
Contributor

No description provided.

Copy link
Contributor

@xluciano xluciano left a comment

Choose a reason for hiding this comment

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

Nice work! I like it is tidy and well commented. I have a few questions. The only important one is about parallelizing the tests.

@@ -339,12 +339,10 @@ float bfp_s16_mean(
headroom_t hr = HR_S32(sum) + 32;
int64_t sum64 = ((int64_t)sum) << hr;
int64_t mean64 = sum64 / ((int)b->length);
right_shift_t shr = MAX(0, 48 - HR_S64(mean64));
right_shift_t shr = MAX(0, 32 - HR_S64(mean64));
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this was the bug you found, well done!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was not a bug but, it's resolving a TODO from below.

if(shr > 0)
mean64 += 1 << (shr-1);
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment to explain why you have to cast a uint32_t value into uint64_t?

Copy link
Contributor Author

@uvvpavel uvvpavel Sep 26, 2023

Choose a reason for hiding this comment

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

I did a slight update on this line. I don't really know how to comment it, as it's a standard procedure for rounding

@@ -335,8 +335,8 @@ float_s32_t bfp_s32_mean(
right_shift_t shr = MAX(0, 32 - HR_S64(mean));

if(shr > 0)
mean += 1 << (shr-1);

mean += (uint32_t)(1 << (shr-1));
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment to explain why you have to cast a uint32_t value into uint64_t?

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 did a slight update on this line. I don't really know how to comment it, as it's a standard procedure for rounding

test/dct_tests/src/test_dctXX_inverse.c Show resolved Hide resolved
steps {
dir('lib_xcore_math/build_xs3a/test') {
withTools(params.TOOLS_VERSION) {
sh 'xrun --xscope --id 0 --args bfp_tests/bfp_tests.xe -v'
Copy link
Contributor

Choose a reason for hiding this comment

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

what does "-v" option do? Doesn't it just print the version? If you want to increase the verbosity, you need to use "--verbose", but I never used it myself,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-v here is for the test itself, not xrun, hence it has --args. It means verbose

#define MAX_PROC_FRAME_LENGTH_LOG2 9
#define MAX_PROC_FRAME_LENGTH (1<<MAX_PROC_FRAME_LENGTH_LOG2)

#define MIN_DCT_N_LOG2 (3)
#define LOOPS_LOG2 (0)



Copy link
Contributor

Choose a reason for hiding this comment

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

why is SMOKE_TEST not used here? Is it because LOOPS_LOG2 is already 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can't go any lower

description: 'The XTC tools version'
)
booleanParam(
name: 'XMATH_SMOKE_TEST',
Copy link
Contributor

Choose a reason for hiding this comment

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

how long the tests last if XMATH_SMOKE_TEST is set to False? I wonder we should use it by default if they last less than 30 min. If we decide to run the tests in smoke test mode, when do we run the full tests?

stages {
stage('Bullds and tests') {
parallel {
stage('Linux builds and tests') {
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't we parallelize the xs3 tests with the x86 ones? We will reduce the duration of the Jenkins run by a considerable amount.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way it's designed right now is that the smoke run takes 4 min, and the non-smoke run takes 30 min. This whole stage is executed on a single agent. It's not gonna bring much benefit for a smoke run. and will reduce a non-smoke run by 4 min as xs3 takes 27 min and x86 takes 4 min.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation and the details

Copy link
Contributor

@xluciano xluciano left a comment

Choose a reason for hiding this comment

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

One more comment.

test/dct_tests/src/test_dctXX_inverse.c Show resolved Hide resolved
Copy link
Contributor

@xluciano xluciano left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor

@xluciano xluciano left a comment

Choose a reason for hiding this comment

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

The Jenkins artifacts contain the build files for the documentation. They shouldn't be archived.
See http://srv-bri-jcim0:8080/blue/organizations/jenkins/XMOS%2Flib_xcore_math/detail/PR-150/18/artifacts

Jenkinsfile Outdated Show resolved Hide resolved
Jenkinsfile Outdated Show resolved Hide resolved
options {
skipDefaultCheckout()
timestamps()
buildDiscarder(xmosDiscardBuildSettings())
Copy link
Contributor

Choose a reason for hiding this comment

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

I should note here that this will modify build deletion settings going forward.
details: https://github.com/xmos/xmos_jenkins_shared_library/pull/331

@BrennanGit
Copy link
Contributor

The Jenkins artifacts contain the build files for the documentation. They shouldn't be archived. See http://srv-bri-jcim0:8080/blue/organizations/jenkins/XMOS%2Flib_xcore_math/detail/PR-150/18/artifacts

This is basically because the docs build "fails". That means it bombs out before the cleanup of build files. I hope to fix this in the v4 modification.

@uvvpavel
Copy link
Contributor Author

The Jenkins artifacts contain the build files for the documentation. They shouldn't be archived. See http://srv-bri-jcim0:8080/blue/organizations/jenkins/XMOS%2Flib_xcore_math/detail/PR-150/18/artifacts

This is basically because the docs build "fails". That means it bombs out before the cleanup of build files. I hope to fix this in the v4 modification.

@lucianomartin I'll leave it there for now as it's being archived with the rest of the docs.

Copy link
Contributor

@xluciano xluciano left a comment

Choose a reason for hiding this comment

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

All good. The doc build artifacts will be removed in a future PR.

Copy link
Contributor

@BrennanGit BrennanGit left a comment

Choose a reason for hiding this comment

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

Jenkins stuff looks good to me 👍

@uvvpavel uvvpavel merged commit bdea3cf into xmos:develop Sep 27, 2023
1 check passed
@uvvpavel uvvpavel deleted the feature/jenkins branch September 27, 2023 09:40
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