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

[microTVM][Zephyr] Fix TVMC test on hardware #13598

Merged
merged 2 commits into from
Dec 14, 2022

Conversation

mehrdadh
Copy link
Member

@mehrdadh mehrdadh commented Dec 12, 2022

This PR adds stack size to run test on hardware for Zephyr. It is not added for Arduino since it is not required.
4096 is derived from experiment on nucleo_l4r5zi.

@tvm-bot
Copy link
Collaborator

tvm-bot commented Dec 12, 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 Author

@tvm-bot rerun

Copy link
Contributor

@gromero gromero left a comment

Choose a reason for hiding this comment

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

Folks, please, the PR description has to be more descriptive. This will help others outside the project, which are not sitting close at the same working table, or attending the same weekly company meeting, to understand and follow how the project evolves or changes over time. It's not just a matter of fixing stuff, but also about sharing information and progress.

Is that hardware a Zephyr HW, an Arduino HW or both? Which board exactly the test is failing and how? I (others) might encounter a similar error on a different board that requires more stack to run the tests in the future and I (and probably others not working close to us) would like to do the due git archaeology to find out hints about how to fix the error! I know it's very simple change but yet there is a lot to be said and shared about it, so, please, add the details to the PR description and, specially, take care to land it neatly with the commit message (also as per the Commit Message Guideline. And please, don't forget the a space between the tags and the title!

--

Alright with adding a default main stack size to run tests on Zephyr, but won't Arduino fail on that since config_main_stack_size is not available on Arduino platform? Hence should a conditional be added in the test to not pass such a option when the Arduino platform is selected?

Cheers.

@mehrdadh
Copy link
Member Author

@gromero sorry about the PR description. I will add more details.
Also thanks for catching the Arduino case, totally missed that. Only if we had hardware-in-loop testing in TVM CI to show these silly mistakes!

Copy link
Contributor

@gromero gromero left a comment

Choose a reason for hiding this comment

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

@mehrdadh Thanks, LGTM!

I think CI error is not related to the change, so maybe just re-trigger.

@gromero
Copy link
Contributor

gromero commented Dec 13, 2022

@tvm-bot rerun

@github-actions
Copy link
Contributor

Failed to re-run CI in https://github.com/apache/tvm/actions/runs/3689290247

Traceback (most recent call last):
  File "/home/runner/work/tvm/tvm/ci/scripts/jenkins/git_utils.py", line 121, in _request
    with request.urlopen(req, data) as response:
  File "/usr/lib/python3.8/urllib/request.py", line 222, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/lib/python3.8/urllib/request.py", line 531, in open
    response = meth(req, response)
  File "/usr/lib/python3.8/urllib/request.py", line 640, in http_response
    response = self.parent.error(
  File "/usr/lib/python3.8/urllib/request.py", line 569, in error
    return self._call_chain(*args)
  File "/usr/lib/python3.8/urllib/request.py", line 502, in _call_chain
    result = func(*args)
  File "/usr/lib/python3.8/urllib/request.py", line 649, in http_error_default
    raise HTTPError(req.full_url, code, msg, hdrs, fp)
urllib.error.HTTPError: HTTP Error 403: Forbidden

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "ci/scripts/github/github_tvmbot.py", line 593, in comment_failure
    raise item
  File "ci/scripts/github/github_tvmbot.py", line 705, in run
    pr.rerun_github_actions()
  File "ci/scripts/github/github_tvmbot.py", line 582, in rerun_github_actions
    raise e
  File "ci/scripts/github/github_tvmbot.py", line 574, in rerun_github_actions
    actions_github.post(f"actions/runs/{workflow_id}/rerun-failed-jobs", data={})
  File "/home/runner/work/tvm/tvm/ci/scripts/jenkins/git_utils.py", line 143, in post
    return self._request(self.base + url, data, method="POST")
  File "/home/runner/work/tvm/tvm/ci/scripts/jenkins/git_utils.py", line 126, in _request
    raise RuntimeError(f"Error response: {msg}\n{error_data}")
RuntimeError: Error response: HTTP Error 403: Forbidden
{"message":"This workflow is already running","documentation_url":"https://docs.github.com/rest/reference/actions#re-run-workflow-failed-jobs"}

@gromero gromero changed the title [microTVM]Fix TVMC test on hardware [microTVM][Zephyr] Fix TVMC test on hardware Dec 13, 2022
@mehrdadh mehrdadh merged commit 949089d into apache:main Dec 14, 2022
@mehrdadh mehrdadh deleted the micro/fix_tvmc_common_test branch December 14, 2022 17:25
@leandron
Copy link
Contributor

Hi all. I think it's important to echo @gromero's comment regarding the Commit Message Guideline. For next PRs, at merging time, please take a moment to adjust the commit message to be more self-explanatory and leave a good track of the changes in our git history.

As I'm involved in the ongoing release process, it complicates a lot the process to be able to track changes that have generic names or no commit message body.

fzi-peccia pushed a commit to fzi-peccia/tvm that referenced this pull request Mar 27, 2023
mikeseven pushed a commit to mikeseven/tvm that referenced this pull request Sep 27, 2023
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.

5 participants