-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Target] Add python binding to new JSON target construction. #6315
Changes from 1 commit
5e83f07
9dd7fe5
f1da53f
0b17562
8c786a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,7 +80,31 @@ def test_target_create(): | |
assert tgt is not None | ||
|
||
|
||
def test_target_config(): | ||
""" | ||
Test that constructing a target from a dictionary works. | ||
""" | ||
target_config = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should also test map-type attributes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you expand on this a little? Do you mean add testing that confirms it fails when invalid attribute types are provided? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just meant we should also test the attribute which type is a map. Since one purpose of testing Python binding is to make sure we can correctly pass supported data structures to the C++ container. Specifically, the Python version of this test: https://github.com/apache/incubator-tvm/blob/master/tests/cpp/target_test.cc#L121 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see. It's a little strange to do this since none of the supported options are maps. But I could add a test case that passes a map and then confirm it fails like the cpp version you linked. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added an analogous test in the latest commit. Let me know if this is what you were looking for. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I saw the map test you added, but it is intented to fail so I'm not sure if it is sufficient. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to be clear, there are no valid arguments that have type map. The tests you linked in |
||
'kind': 'llvm', | ||
'keys': ['arm_cpu', 'cpu'], | ||
'device': 'arm_cpu', | ||
'libs': ['cblas'], | ||
'system-lib': True, | ||
'mfloat-abi': 'hard', | ||
'mattr': ['+neon', '-avx512f'], | ||
} | ||
target = tvm.target.create(target_config) | ||
assert target.kind.name == 'llvm' | ||
assert all([key in target.keys for key in ['arm_cpu', 'cpu']]) | ||
assert target.device_name == 'arm_cpu' | ||
assert target.libs == ['cblas'] | ||
assert 'system-lib' in str(target) | ||
assert target.attrs['mfloat-abi'] == 'hard' | ||
assert all([attr in target.attrs['mattr'] for attr in ['+neon', '-avx512f']]) | ||
|
||
|
||
if __name__ == "__main__": | ||
test_target_dispatch() | ||
test_target_string_parse() | ||
test_target_create() | ||
test_target_config() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we also do some guessing work as well for strings? If it is a string but seems like a JSON, we can use
json.loads
instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to support the following use case as well:
This case could be supported by using
os
to check if the string is a file path or not.In addition, if we would like to support the JSON string, I personally don't think checking the first character is a proper approach, as you may have spaces or something else. A better way should be just trying to parse the string with JSON parser first and use string parser instead if the JSON parser throws an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we should try to find a unified approach to create target from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that it is one line to open and read the file. I think we can just allow it takes in string and not support the file path feature(because supporting both could be confusing). It also helps to avoid conflcits(e.g. what if there is a file with the same name).
target = target.create(open("target.json").read())
is not as badThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a try except block to see if the input string can be parsed to json. We now can accept traditional target string, python dicts, or json strings. Let me know we need other options like tag (not sure what this means).