[CI] Upgrade CI#14635
Conversation
|
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 |
|
cc @masahi |
leandron
left a comment
There was a problem hiding this comment.
Hi @yongwww, thanks for the PR. I see this is marked as work in progress.
Can I just ask the we validate the images in a full CI round before merging this PR, that not only they build the images correctly, but also that tests pass with the resultant images, as you're doing with #14651. These are a lot of changes for one PR, but I appreciate that moving versions is hard.
I'm marking this as "request changes" so that we don't accidentally merge this based on the local CI results.
|
Hi @yongwww, thanks for the PR. I saw you tried tensorflow==2.12.0 but it was rolled back. Have you considered tensorflow==2.10? For example, Vela 3.7.0 (recently updated) supports TensorFlow 2.10. |
|
@Aleksei-grovety great observation! I rolled back the TensorFlow upgrade because the linking process failed for the TFLite runtime during the TVM build (lots of errors like "multiple definition ...", and "undefined reference to..."). This issues need specific update in the tflite runtime build, which I haven't had time to address. In fact, I think we should consider using TFLite runtime Python package (https://pypi.org/project/tflite-runtime/) provided by Google. |
|
@yongwww Feel free to drop PT 2.0 update from this PR. I'm aware that some PT frontend tests are broken with 2.0. I can continue my PR to fix them, once we get Python 3.10 from here. |
|
CI #14651 is green |
|
Thank you @yongwww for heroic effort! |
|
cc @leandron can you take another look at this PR? I think this is ready to be merged. |
|
ping @leandron |
|
@tvm-bot rerun |
cautiously dismissing since the comments have been addressed and this PR has been in flight for a while, we can address any concerns in follow ups
leandron
left a comment
There was a problem hiding this comment.
I'm happy to merge this once #14651 CI is green. At the moment it is running, as the GitHub actions needed to be approved, which I just did.
I marked a few action I feel we need to take as follow-up PRs, can you just open those as issues so that we can pick them up asap. There no need to block this PR because the actual changes, but can you open the issues, please?
|
|
||
| cd "$repo_dir"/driver | ||
| scons install_prefix="$install_path" install | ||
| scons werror=False install_prefix="$install_path" install |
There was a problem hiding this comment.
Do you mind opening an issue to report what is the error that makes this script now need werror=False?
I understand it will have some output, but can you add that output in the issue, so that somebody can pick that up to investigate?
There was a problem hiding this comment.
I got error like "error: loop variable 'pair' creates a copy from type 'const std::pair<const unsigned int, std::pair<long unsigned int, long unsigned int> >' [-Werror=range-loop-construct] for (const auto pair : passAgentIdRanges)", it should be a warning, so I added werror=False in scons install for that, I think the ideal solution is to modify the code in https://github.com/Arm-software/ethos-n-driver-stack to fix this warning. More details about the error info. please refer to the log https://ci.tlcpack.ai/blue/rest/organizations/jenkins/pipelines/tvm-docker/branches/PR-14635/runs/5/nodes/73/steps/129/log/?start=0 Probably we can create an issue in the arm repo?
|
Going to merge this first in 24 hours so we can proceed with the updates on the other side |
|
Thank you @yongwww for heroic effort |
Uh oh!
There was an error while loading. Please reload this page.