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.

Sign up for GitHub

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

Jump to bottom

[BYOC][ACL] Enable remote device via environment variables #6279

Merged
merged 4 commits into from
Aug 25, 2020
Merged

[BYOC][ACL] Enable remote device via environment variables #6279

merged 4 commits into from
Aug 25, 2020

Conversation

lhutton1
Copy link
Contributor

Improves the ACL remote testing infrastructure by allowing a remote device to be specified via environment variables. This means external scripts can be used to enable the runtime tests. By default an RPC server will not be used and the runtime tests will be skipped.

cc @leandron @u99127 @comaniac @zhiics

@comaniac
Copy link
Contributor

TVM_ARM_RPC_* is confusing, especially it is only used by the testing infra. IMHO, TVM_TEST_ARM_RPC_* would be slightly better. On the other hand, TVM_RPC_* seems too general. I don't think we need a series of TVM_RPC_* environment variables to control the RPC setting else where.

Meanwhile, I am not a fan of this style to be honest. From the unit test and CI point of view we should not depend on environment variables if we don't have to. Would like to hear from other's opinions.

@lhutton1
Copy link
Contributor Author

lhutton1 commented Aug 21, 2020 •
edited
Loading

Apologies for the delay following up on this. I agree this isn't the most elegant, I think our options are limited though. One idea I did have was to check for the presence of an arm-compute-lib-tests.config file in the test_arm_compute_lib directory. If it exists we can open it and parse the values. Maybe we could also check PATH as well if the file is placed elsewhere?

Open to other suggestions though :)

@comaniac
Copy link
Contributor

This solution is much better for sure, as it still makes ACL test infra self-contained.

cc @zhiics for comments.

@zhiics
Copy link
Member

zhiics commented Aug 22, 2020

@u99127, @leandron, please take another look and approve or comment as per https://tvm.apache.org/docs/contribute/code_review.html#approve-and-request-changes-explicitly

@lhutton1
[BYOC][ACL] Enable remote device via environment variables
32edf55 
Improves the ACL remote testing infrastructure by allowing a remote device to be specified via environment variables. This means external scripts can be used to enable the runtime tests. By default an RPC server will not be used and the runtime tests will be skipped.

Change-Id: I8fc0b88106683ac6f1cbff44c8954726325cda21
@lhutton1
Use json file as configuration for tests
a75776c 
Change-Id: Iadce931d91056ed3a2d57a49f14af1ce771ae14b
@lhutton1
Do not load the test config during class creation
66ff00f 
Change-Id: If718b5d163e399711111830f878db325db9c5f84
comaniac
comaniac approved these changes Aug 24, 2020
View reviewed changes
Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

LGTM. Just a nit.

tests/python/contrib/test_arm_compute_lib/infrastructure.py Outdated Show resolved Hide resolved
@lhutton1
Add check for existence of file
e92c09e 
Change-Id: I2568bca7f4c3ad22ee8f9d065a9486ee3114f35c
@zhiics zhiics merged commit 0466d60 into apache:master Aug 25, 2020
@zhiics
Copy link
Member

zhiics commented Aug 25, 2020

Thanks @lhutton1 @comaniac @leandron

trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
@lhutton1
[BYOC][ACL] Enable remote device via environment variables (apache#6279)
22f6751 
* [BYOC][ACL] Enable remote device via environment variables

Improves the ACL remote testing infrastructure by allowing a remote device to be specified via environment variables. This means external scripts can be used to enable the runtime tests. By default an RPC server will not be used and the runtime tests will be skipped.

Change-Id: I8fc0b88106683ac6f1cbff44c8954726325cda21

* Use json file as configuration for tests

Change-Id: Iadce931d91056ed3a2d57a49f14af1ce771ae14b

* Do not load the test config during class creation

Change-Id: If718b5d163e399711111830f878db325db9c5f84

* Add check for existence of file

Change-Id: I2568bca7f4c3ad22ee8f9d065a9486ee3114f35c
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
@lhutton1
[BYOC][ACL] Enable remote device via environment variables (apache#6279)
34c0ba4 
* [BYOC][ACL] Enable remote device via environment variables

Improves the ACL remote testing infrastructure by allowing a remote device to be specified via environment variables. This means external scripts can be used to enable the runtime tests. By default an RPC server will not be used and the runtime tests will be skipped.

Change-Id: I8fc0b88106683ac6f1cbff44c8954726325cda21

* Use json file as configuration for tests

Change-Id: Iadce931d91056ed3a2d57a49f14af1ce771ae14b

* Do not load the test config during class creation

Change-Id: If718b5d163e399711111830f878db325db9c5f84

* Add check for existence of file

Change-Id: I2568bca7f4c3ad22ee8f9d065a9486ee3114f35c
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Aug 26, 2020
@lhutton1
[BYOC][ACL] Enable remote device via environment variables (apache#6279)
e78d262 
* [BYOC][ACL] Enable remote device via environment variables

Improves the ACL remote testing infrastructure by allowing a remote device to be specified via environment variables. This means external scripts can be used to enable the runtime tests. By default an RPC server will not be used and the runtime tests will be skipped.

Change-Id: I8fc0b88106683ac6f1cbff44c8954726325cda21

* Use json file as configuration for tests

Change-Id: Iadce931d91056ed3a2d57a49f14af1ce771ae14b

* Do not load the test config during class creation

Change-Id: If718b5d163e399711111830f878db325db9c5f84

* Add check for existence of file

Change-Id: I2568bca7f4c3ad22ee8f9d065a9486ee3114f35c
trevor-m pushed a commit to trevor-m/tvm that referenced this pull request Sep 2, 2020
@lhutton1
[BYOC][ACL] Enable remote device via environment variables (apache#6279)
d961c24 
* [BYOC][ACL] Enable remote device via environment variables

Improves the ACL remote testing infrastructure by allowing a remote device to be specified via environment variables. This means external scripts can be used to enable the runtime tests. By default an RPC server will not be used and the runtime tests will be skipped.

Change-Id: I8fc0b88106683ac6f1cbff44c8954726325cda21

* Use json file as configuration for tests

Change-Id: Iadce931d91056ed3a2d57a49f14af1ce771ae14b

* Do not load the test config during class creation

Change-Id: If718b5d163e399711111830f878db325db9c5f84

* Add check for existence of file

Change-Id: I2568bca7f4c3ad22ee8f9d065a9486ee3114f35c
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Sep 3, 2020
@lhutton1 @trevor-m
[BYOC][ACL] Enable remote device via environment variables (apache#6279)
7ce9760 
* [BYOC][ACL] Enable remote device via environment variables

Improves the ACL remote testing infrastructure by allowing a remote device to be specified via environment variables. This means external scripts can be used to enable the runtime tests. By default an RPC server will not be used and the runtime tests will be skipped.

Change-Id: I8fc0b88106683ac6f1cbff44c8954726325cda21

* Use json file as configuration for tests

Change-Id: Iadce931d91056ed3a2d57a49f14af1ce771ae14b

* Do not load the test config during class creation

Change-Id: If718b5d163e399711111830f878db325db9c5f84

* Add check for existence of file

Change-Id: I2568bca7f4c3ad22ee8f9d065a9486ee3114f35c
@lhutton1 lhutton1 deleted the acl-remote-testing branch September 23, 2020 10:27
@ZihengJiang ZihengJiang mentioned this pull request Sep 25, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@u99127 u99127 u99127 left review comments

@leandron leandron leandron left review comments

@comaniac comaniac comaniac approved these changes

@zhiics zhiics zhiics approved these changes

Assignees

@zhiics zhiics

@comaniac comaniac

Labels
status: accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lhutton1 @comaniac @zhiics @leandron @u99127 @ZihengJiang