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] Split Jenkinsfile into platform-specific jobs #13300

Merged
merged 5 commits into from
Dec 6, 2022

Conversation

driazati
Copy link
Member

@driazati driazati commented Nov 4, 2022

This breaks up the Jenkinsfile into ones for GPU, CPU, etc. This removes a false dependency between the build and test steps (e.g. before the GPU tests had to wait on the Hexagon build to complete) and makes the Jenkins UI a bit better since there's not 30 tests to scroll through to find a failure. An example can be found in my fork here: driazati#38 in the checks box. Before this is merged https://github.com/tlc-pack/ci/blob/main/jenkins/jenkins-jobs/prod/tvm.yaml will need to be updated to accept webhooks from apache/tvm instead of my fork.

See #13337 for more context

@tvm-bot
Copy link
Collaborator

tvm-bot commented Nov 4, 2022

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

Generated by tvm-bot

@mehrdadh
Copy link
Member

mehrdadh commented Nov 7, 2022

This is great!!

@quic-sanirudh
Copy link
Contributor

This is amazing. It'll make it so much easier to find failures, thanks a lot for this.

@driazati driazati marked this pull request as ready for review November 7, 2022 17:35
driazati added a commit to driazati/tvm that referenced this pull request Nov 7, 2022
This is a follow on to tlc-pack/ci#58 and apache#13300. This removes the old `Jenkinsfile` and makes the new job statuses required for PRs to merge.
@mehrdadh
Copy link
Member

mehrdadh commented Nov 8, 2022

Another great point about this PR is that now we can easily replicate only a part of the CI in a separate environment. For example, if someone wants to only run Hexagon tests or microTVM tests on physical devices, they could reuse the same Jenkins file for cortexm or hexagon.

ci/jenkins/generate.py Outdated Show resolved Hide resolved
args = parser.parse_args()

sources = TEMPLATES_DIR.glob("*jenkinsfile.groovy.j2")
changes = [update_jenkinsfile(source) for source in sources if source.name != "base.groovy.j2"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if the if condition is necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

its just there to keep from copying base.groovy.j2 to the generated directory since it doesn't do anything outside of provide helper functions

ci/jenkins/generate.py Outdated Show resolved Hide resolved
)
skip_ci = should_skip_ci(env.CHANGE_ID)
skip_slow_tests = should_skip_slow_tests(env.CHANGE_ID)
rebuild_docker_images = sh (
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see rebuild_docker_images is set and never used in all the generated Jenkins files except the docker one. I don't think this is a particular issue at the moment. Even though it would be nice to have only the current image name used (ci_arm in this case).

However, it makes me wonder if having a separate job for building the docker images allows for a race between a job trying to pull a docker image and the docker_jenkisfile building a new docker image.

Should we maybe have instead each platform (arm, cpu, gpu ...) build its own image if needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now the image build is just to check the build so it doesn't actually use them later on. We should definitely come in and try to clean this up some more later on since it's still a bit tangled up with the old monolithic job

ci/jenkins/generated/arm_jenkinsfile.groovy Outdated Show resolved Hide resolved
ci/jenkins/generated/arm_jenkinsfile.groovy Outdated Show resolved Hide resolved
)
}

stage('Docker Image Build') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be outside of a method. Are we ok with that?

Copy link
Member Author

Choose a reason for hiding this comment

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

As long as Jenkins doesn't blow up when running it's ok (the JVM has a 64k limit on method size or something so if there's too much generated code things need to be split up)

ci/jenkins/generated/docker_jenkinsfile.groovy Outdated Show resolved Hide resolved
ci/jenkins/generated/lint_jenkinsfile.groovy Show resolved Hide resolved
{% include "ci/jenkins/Test.groovy.j2" %}
{% include "ci/jenkins/Deploy.groovy.j2" %}
{% include "utils/Prepare.groovy.j2" %}
{% include "utils/Build.groovy.j2" %}


cancel_previous_build()

prepare()
Copy link
Contributor

Choose a reason for hiding this comment

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

We could add a newline here

Copy link
Member Author

Choose a reason for hiding this comment

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

The EOF-adjacent newlines get caught by https://github.com/apache/tvm/blob/main/tests/lint/whitespace.sh, so we can't really change them without adding an exception

@driazati driazati requested review from gigiblender and areusch and removed request for gigiblender November 18, 2022 22:26
Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

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

thanks @driazati, couple questions but i agree with splitting these up

ci/jenkins/data.py Outdated Show resolved Hide resolved
docker/dev_common.sh Outdated Show resolved Hide resolved
ci/scripts/jenkins/open_docker_update_pr.py Outdated Show resolved Hide resolved
ci/jenkins/templates/utils/Build.groovy.j2 Show resolved Hide resolved
ci/jenkins/templates/utils/Build.groovy.j2 Show resolved Hide resolved
ci/jenkins/generated/arm_jenkinsfile.groovy Show resolved Hide resolved
@driazati driazati force-pushed the jenkins_jobs branch 2 times, most recently from a24d29a to dac62df Compare December 2, 2022 01:23
Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

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

thanks @driazati a couple small last things

docker/dev_common.sh Outdated Show resolved Hide resolved
@driazati driazati force-pushed the jenkins_jobs branch 3 times, most recently from 40627e8 to 9f5ec45 Compare December 6, 2022 06:37
Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

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

thanks @driazati !

@driazati driazati merged commit 9374738 into apache:main Dec 6, 2022
@shingjan
Copy link
Contributor

shingjan commented Dec 6, 2022

thanks @driazati ! Great work!

driazati added a commit that referenced this pull request Dec 7, 2022
This is a follow on to tlc-pack/ci#58 and #13300. This removes the old `Jenkinsfile` and makes the new job statuses required for PRs to merge.
driazati added a commit to driazati/tvm that referenced this pull request Dec 7, 2022
The PR apache#13300 had a bad merge with main an undid a fix from apache#13442, this
adds it back in.
@driazati driazati mentioned this pull request Dec 7, 2022
driazati added a commit to driazati/tvm that referenced this pull request Dec 7, 2022
The PR apache#13300 had a bad merge with main an undid a fix from apache#13442, this
adds it back in.
driazati added a commit to driazati/tvm that referenced this pull request Dec 7, 2022
The PR apache#13300 had a bad merge with main an undid a fix from apache#13442, this
adds it back in.
Mousius pushed a commit that referenced this pull request Dec 7, 2022
The PR #13300 had a bad merge with main an undid a fix from #13442, this
adds it back in.
@alter-xp
Copy link
Contributor

hi @driazati , It seems that the contents related to how to regenerate Jenkinsfile in ci/README.md and ci/jenkins/README.md are already out of date. Is there any new tutorial?

@driazati
Copy link
Member Author

hi @driazati , It seems that the contents related to how to regenerate Jenkinsfile in ci/README.md and ci/jenkins/README.md are already out of date. Is there any new tutorial?

Ah good catch, I will follow up with a PR to update the docs but you can run this to re-sync everything

python3 -m pip install jinja2
python ci/jenkins/generate.py

fzi-peccia pushed a commit to fzi-peccia/tvm that referenced this pull request Mar 27, 2023
The PR apache#13300 had a bad merge with main an undid a fix from apache#13442, this
adds it back in.
mikeseven pushed a commit to mikeseven/tvm that referenced this pull request Sep 27, 2023
This breaks up the Jenkinsfile into ones for GPU, CPU, etc. This removes a false dependency between the build and test steps (e.g. before the GPU tests had to wait on the Hexagon build to complete) and makes the Jenkins UI a bit better since there's not 30 tests to scroll through to find a failure. An example can be found in my fork here: driazati#38 in the checks box. Before this is merged https://github.com/tlc-pack/ci/blob/main/jenkins/jenkins-jobs/prod/tvm.yaml will need to be updated to accept webhooks from apache/tvm instead of my fork.

See apache#13337 for more context
mikeseven pushed a commit to mikeseven/tvm that referenced this pull request Sep 27, 2023
…he#13316)

This is a follow on to tlc-pack/ci#58 and apache#13300. This removes the old `Jenkinsfile` and makes the new job statuses required for PRs to merge.
mikeseven pushed a commit to mikeseven/tvm that referenced this pull request Sep 27, 2023
The PR apache#13300 had a bad merge with main an undid a fix from apache#13442, this
adds it back in.
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.

8 participants