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] Add llvm-15 and mlir-15 to Docker setup #14303

Merged
merged 1 commit into from
Apr 1, 2023

Conversation

multiverstack-intellif
Copy link
Contributor

@multiverstack-intellif multiverstack-intellif commented Mar 15, 2023

Add llvm-15 and mlir-15 for CPU docker image for RFC: Introduce PresburgerSet

libstdc++ needs to be updated for ubuntu-18.04 if install llvm-15 through the official apt, which could be risky. So here build it from source as a temporary solution before overall upgrade to ubuntu-22.04.

cc @leandron @lhutton1 @tqchen

@tvm-bot
Copy link
Collaborator

tvm-bot commented Mar 15, 2023

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

@multiverstack-intellif multiverstack-intellif changed the title Add llvm-15 and mlir-15 to Docker setup. [CI] Add llvm-15 and mlir-15 to Docker setup. Mar 15, 2023
@multiverstack-intellif multiverstack-intellif changed the title [CI] Add llvm-15 and mlir-15 to Docker setup. [CI] Add llvm-15 and mlir-15 to Docker setup Mar 15, 2023
@multiverstack-intellif
Copy link
Contributor Author

multiverstack-intellif commented Mar 15, 2023

@tvm-bot rerun

Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

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

We had a look in the possibility of adding LLVM-15 using apt.llvm.org, but it seems a bit risky due to the need to update libstdc++ globally in that image. Yesterday I pushed a PR that did the formal update in #14296.

As an alternative that doesn't require changing libstdc++, you can use the script that was introduced in #13823, which is currently in use in ci_arm. Would you consider using that, so that we can have the same version in both images, favouring consistency across different images? If more parts of LLVM needs to be build, it would be good to have that in all images as well.

Also as another datapoint, building LLVM from source is also used in the tlcpack packaging scripts, which would also be aligned with building LLVM from source.

(I noticed your PR is draft, but just wanted to leave this comment to be considered before we merge this)

also cc @lhutton1 who might be interested in this.

@tqchen
Copy link
Member

tqchen commented Mar 15, 2023

I feel it is still helpful to be able to rely on the official apt for some(wasn't aware of the source change).

This is mainly because most people do not have the capacity necessarily build LLVM from scratch and being able to rely on official ones helps to reduce that burden.

Perhaps we can resolve problem of libstdc++ dependency seems can be resolved by using the right version of the compiler.

@leandron
Copy link
Contributor

I feel it is still helpful to be able to rely on the official apt for some(wasn't aware of the source change).

This is mainly because most people do not have the capacity necessarily build LLVM from scratch and being able to rely on official ones helps to reduce that burden.

I agree. I think the problem goes away in newer versions of Ubuntu according to apt.llvm.org. Once we update Ubuntu in our CI, I think we should go with packages as we always did, for now I'd recommend building it from source.

@tqchen
Copy link
Member

tqchen commented Mar 15, 2023

@lhutton1
Copy link
Contributor

Thanks for the discussion, following the discussion mentioned above, would be worth installing LLVM15 from source in the meantime while the prerequisite upgrades are carried out?

@leandron
Copy link
Contributor

Hi. Friendly nudge so that we can take a decision on how to proceed here. I'd suggest that we build LLVM 15 from source, similar to what is being done in ci_arm for now. Once we move to Ubuntu 22, then we can just go back to the APT repository, mostly because of the global libc++ change which might affect all other builds we do.

If we agree on this strategy, we have the option to change this PR or submitting a new one with LLVM build from source, which is equally simple as a PR. Happy to help.

@lhutton1 @multiverstack-intellif @tqchen what do you think?

@multiverstack-intellif
Copy link
Contributor Author

Hi. Friendly nudge so that we can take a decision on how to proceed here. I'd suggest that we build LLVM 15 from source, similar to what is being done in ci_arm for now. Once we move to Ubuntu 22, then we can just go back to the APT repository, mostly because of the global libc++ change which might affect all other builds we do.

If we agree on this strategy, we have the option to change this PR or submitting a new one with LLVM build from source, which is equally simple as a PR. Happy to help.

@lhutton1 @multiverstack-intellif @tqchen what do you think?

I think building from source is a good temporary solution for this, since the overall upgrade may take weeks or even months to get done. I'll change the PR accordingly and ask for another review if no objection.

@github-actions github-actions bot requested review from lhutton1 and tqchen March 30, 2023 09:17
@multiverstack-intellif multiverstack-intellif marked this pull request as ready for review March 30, 2023 09:19
Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

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

@tqchen
Copy link
Member

tqchen commented Mar 30, 2023

happy to go with what @multiverstack-intellif you think is convenient

@wrongtest-intellif wrongtest-intellif merged commit 49e6695 into apache:main Apr 1, 2023
@multiverstack-intellif
Copy link
Contributor Author

@leandron I'm asking just in case you may know. The change is merged, but I only found image tlcpackstaging/ci_cpu in daily docker build(https://ci.tlcpack.ai/job/docker-images-ci/job/daily-docker-image-rebuild/). Do you have any idea how tlcpack/ci_cpu is built? Tag of tlcpack/ci_cpu is expected to be finally used, right? Thanks very much!

@lhutton1
Copy link
Contributor

lhutton1 commented Apr 3, 2023

Hi @multiverstack-intellif, you should be able to post a PR with the tlcpackstaging image tag but under the tlcpack account, see https://tvm.apache.org/docs/contribute/ci.html#updating-a-docker-image-tag

junrushao pushed a commit that referenced this pull request Apr 11, 2023
Update docker image tag as a follow up step for #14303.
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.

7 participants