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

[Hexagon] Pytestify Hexagon unit test #8955

Merged
merged 5 commits into from Sep 23, 2021
Merged

[Hexagon] Pytestify Hexagon unit test #8955

merged 5 commits into from Sep 23, 2021

Conversation

kparzysz-quic
Copy link
Contributor

No description provided.

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 @kparzysz-quic , just one question

hexagon.register_linker(lambda: "/bin/true")
return True
# Register a phony linker, so that we can test codegen without a Hexagon toolchain.
hexagon.register_linker(lambda: "/bin/true")
Copy link
Contributor

Choose a reason for hiding this comment

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

should this become a fixture and unregister on test exit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that would be cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One problem is that if all of these tests run from a common "super"-module, and are executed in parallel, then the linker may get unregistered by one test before the other tests are done using it. Right now this is the only tests that does this, but if there are more added later, then it may become an issue.

Another thing may be that we may want to do lazy linking in Hexagon module, so that we're not looking for the linker each time when tvm.build is executed...

Copy link
Contributor

Choose a reason for hiding this comment

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

pytest-xdist will spawn new sys.executable to distribute the load, so shouldn't need to worry about refcounting here. the fixture should be enough. i am going to push on the xdist PR again soon, which is why i ask :). do you mind making the change here? it's not too complicated--just @pytest.fixture around a local function, move the register call there, and take the function name as an argument to each test_

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've turned it into a fixture.

@areusch
Copy link
Contributor

areusch commented Sep 20, 2021

@kparzysz-quic does my request seem reasonable to you?

@kparzysz-quic
Copy link
Contributor Author

I added the yield expression with unregistration.

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 @kparzysz-quic ! please resubmit as the CI is fixed.

@kparzysz-quic kparzysz-quic merged commit a2bf430 into apache:main Sep 23, 2021
@kparzysz-quic kparzysz-quic deleted the pytest-hexagon-test branch September 23, 2021 00:11
ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
* [Hexagon] Pytestify Hexagon unit test

* Fix formatting

* Convert linker registration into pytest fixture

* Use yield in pytest fixture

* Restart CI
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
* [Hexagon] Pytestify Hexagon unit test

* Fix formatting

* Convert linker registration into pytest fixture

* Use yield in pytest fixture

* Restart CI
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.

None yet

4 participants