[Target] Add python binding to new JSON target construction.

[jwfromm]

This PR adds the ability to create targets from a configuration dictionary. It's a very thin wrapper around the c++ version of this feature introduced in #6218.

[jwfromm]

@junrushao1994 can you review this PR?

[tqchen]

also cc @zhiics @comaniac @yzhliu @leandron

[junrushao]

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.

Suggested change

	if isinstance(target, str):
	return _ffi_api.TargetFromString(target)
	if isinstance(target, str):
	if target.startswith("{"):
	return _ffi_api.TargetFromConfig(json.loads(target))
	else:
	return _ffi_api.TargetFromString(target)

[comaniac]

I would prefer to support the following use case as well:

target = target.create('./target.json') # Input is a string file path.

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.

[junrushao]

Yes, I think we should try to find a unified approach to create target from

1. file
2. json string
3. json-like dict
4. legacy raw string
5. tag

[tqchen]

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 bad

[jwfromm]

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).

[comaniac]

I would prefer to support the following use case as well:

target = target.create('./target.json') # Input is a string file path.

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.

[comaniac]

Should also test map-type attributes.

[jwfromm]

Can you expand on this a little? Do you mean add testing that confirms it fails when invalid attribute types are provided?

[comaniac]

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

[jwfromm]

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.

[jwfromm]

Added an analogous test in the latest commit. Let me know if this is what you were looking for.

[comaniac]

I saw the map test you added, but it is intented to fail so I'm not sure if it is sufficient.
@junrushao1994 could you comment?

[jwfromm]

Just to be clear, there are no valid arguments that have type map. The tests you linked in target_test.cc also are intentionally failing.

[junrushao]

It looks good from your side :-)

[jwfromm]

@tqchen, tests are passing and reviewers are happy, can we merge this?

[leandron]

I tested it integrating with my ongoing PR #6302 and it works.

One small comment is that I didn't see here a test checking that plain JSON strings, such as { "kind": "llvm" }. Would that make sense to add one, to guarantee the internal behaviour is validated, and the _ffi_api.TargetFromConfig branch is also covered? Let me know in case I'm missing something. (PS: maybe on a separate patch, so that it doesn't block your merge)

edit: found it now.