Skip to content

Combine unit and integration test steps into one stage #9733

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.

Sign up for GitHub

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
Mousius merged 2 commits into from
Jan 11, 2022
Merged

Combine unit and integration test steps into one stage #9733

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d97bd25
Combine unit and integration test steps into one stage
driazati Dec 13, 2021
5c4a6fa
blank
driazati Dec 14, 2021
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
46 changes: 29 additions & 17 deletions Jenkinsfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,16 +267,9 @@ stage('Build') {
pack_lib('cpu', tvm_multilib_tsim)
timeout(time: max_time, unit: 'MINUTES') {
ci_setup(ci_cpu)
python_unittest(ci_cpu)
fsim_test(ci_cpu)
sh (
script: "${docker_run} ${ci_cpu} ./tests/scripts/task_python_vta_tsim.sh",
label: "Run VTA tests in TSIM",
)
// sh "${docker_run} ${ci_cpu} ./tests/scripts/task_golang.sh"
// TODO(@jroesch): need to resolve CI issue will turn back on in follow up patch
sh (script: "${docker_run} ${ci_cpu} ./tests/scripts/task_rust.sh", label: "Rust build and test")
junit "build/pytest-results/*.xml"
}
}
}
Expand Down Expand Up @@ -367,8 +360,8 @@ stage('Build') {
}
}

stage('Unit Test') {
parallel 'python3: GPU': {
stage('Test') {
parallel 'unittest: GPU': {
if (is_docs_only_build != 1) {
node('TensorCore') {
ws(per_exec_ws('tvm/ut-python-gpu')) {
Expand Down Expand Up @@ -397,10 +390,10 @@ stage('Unit Test') {
}
}
} else {
Utils.markStageSkippedForConditional('python3: GPU')
Utils.markStageSkippedForConditional('unittest: GPU')
}
},
'python3: CPU': {
Copy link
Contributor

@areusch areusch Jan 3, 2022

Choose a reason for hiding this comment

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

i think right now, failing unit tests wouldn't cancel integration tests. are we concerned with overburdening CI with PRs that fail unit tests? I wonder if we should somehow cancel integration test builds if unit tests fail. we could also just try merging and see if it's a problem, too.

Copy link
Member Author

@driazati driazati Jan 3, 2022

Choose a reason for hiding this comment

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

The potential time savings of this are pretty significant so I think it's worth it even if it demands more capacity from CI. I could try to pull some numbers of # of jobs vs # of jobs with failing unit tests vs # of jobs with failing integration tests to back this up

Copy link
Member Author

@driazati driazati Jan 5, 2022

Choose a reason for hiding this comment

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

Here are some classifications over the last several failing PR jobs, seems like the unit test failures aren't super frequent compared to others so this PR is probably fine in terms of demand increase

build	262
lint	91
integration	46
infra	22
unit tests	19
unclassified	3

Copy link
Contributor

@areusch areusch Jan 6, 2022

Choose a reason for hiding this comment

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

ok, main concern here is about the frontend tests i think. those do take over an hour to run, so that might be a big load increase if they don't get cancelled.

Copy link
Member Author

@driazati driazati Jan 6, 2022

Choose a reason for hiding this comment

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

sorry should have included this, but this is how I did the accounting:

Details 
WHEN name like '%Build and run C++ tests' THEN 'build'
WHEN name like '%Run cmake build' THEN 'build'
WHEN name like '%Run microTVM tests' THEN 'build'
WHEN name like '%integration tests' THEN 'integration'
WHEN name like '%frontend tests' THEN 'integration'
WHEN name like '%unit tests' THEN 'unit tests'
WHEN name like '%Sphinx warnings in docs' THEN 'lint'
WHEN name like '%Run lint' THEN 'lint'
WHEN name like '%executor node info' THEN 'infra'
WHEN name like '%Check out from version control' THEN 'infra'
WHEN name like '%JUnit-formatted test results' THEN 'infra'
WHEN name like '%Docker image names' THEN 'infra'
WHEN name like '%files previously stashed' THEN 'infra'
WHEN name like '%Rust build and test' THEN 'build'

Isn't the concern the case where unit tests that would have failed and then caused frontend (integration) tests to not run, so the number to look at is unit test failures (since with this PR those would no longer be gating frontend failures)? Frontend failures would mean that the build got all the way there anyways so the capacity requirements of those would be the same

Copy link
Contributor

@areusch areusch Jan 11, 2022

Choose a reason for hiding this comment

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

ok, given the data i support trying this change! we may need to evaluate its impact as folks start using it day-to-day.

'integration: CPU': {
if (is_docs_only_build != 1) {
node('CPU') {
ws(per_exec_ws("tvm/ut-python-cpu")) {
Expand All @@ -417,7 +410,29 @@ stage('Unit Test') {
}
}
} else {
Utils.markStageSkippedForConditional('python3: CPU')
Utils.markStageSkippedForConditional('integration: CPU')
}
},
'unittest: CPU': {
if (is_docs_only_build != 1) {
node('CPU') {
ws(per_exec_ws("tvm/ut-python-cpu")) {
init_git()
unpack_lib('cpu', tvm_multilib_tsim)
timeout(time: max_time, unit: 'MINUTES') {
ci_setup(ci_cpu)
python_unittest(ci_cpu)
fsim_test(ci_cpu)
sh (
script: "${docker_run} ${ci_cpu} ./tests/scripts/task_python_vta_tsim.sh",
label: "Run VTA tests in TSIM",
)
junit "build/pytest-results/*.xml"
}
}
}
} else {
Utils.markStageSkippedForConditional('unittest: CPU')
}
},
'python3: i386': {
Expand Down Expand Up @@ -463,11 +478,8 @@ stage('Unit Test') {
} else {
Utils.markStageSkippedForConditional('python3: arm')
}
}
}

stage('Integration Test') {
Copy link
Contributor

@areusch areusch Jan 3, 2022

Choose a reason for hiding this comment

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

however I'm not sure it's a good idea to run the frontend tests as non-cancellable without unit tests passing

Copy link
Contributor

@areusch areusch Jan 11, 2022

Choose a reason for hiding this comment

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

let's give this a shot and we can always bring forward the failFast logic from @Mousius now that we are going this route

parallel 'topi: GPU': {
},
'topi: GPU': {
if (is_docs_only_build != 1) {
node('GPU') {
ws(per_exec_ws('tvm/topi-python-gpu')) {
Expand Down