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

[AutoTVM] Fix database APIs #3821

Merged
merged 2 commits into from
Aug 28, 2019
Merged

[AutoTVM] Fix database APIs #3821

merged 2 commits into from
Aug 28, 2019

Conversation

comaniac
Copy link
Contributor

The database APIs designed in AutoTVM will throw exceptions with Redis database in Python 3. The main reason is that Python 3 package redis returns values in bytes format instead of str so it cannot be manipulated by builtin string methods like split.

The original code uses str(result) to convert bytes to str, but this is not the right way:

bval = b"some strings" # type(bval) -> <class 'byte'>
sval = str(bval) # type(sval) -> <class 'str'>
print(sval)
'b\'some strings\''

As can be seen, str simply invokes bytes.__str__() and results in unnecessary b\'...\' characters. The right way to do so is using decode method:

dval = bval.decode() # type(dval) -> <class 'str'>
print(dval)
'some strings'

This PR fixes the issue and creates a new unit test to cover filer method which wasn't covered before. With the changes, the unit test passes for both DummyDatabase and RedisDatabase locally.

One remain issue could be the lack of unit tests for Redis database, but this requires changes of CI environment.

@comaniac
Copy link
Contributor Author

@icemelon9 please help review and merge.

python/tvm/autotvm/database.py Show resolved Hide resolved
python/tvm/autotvm/database.py Outdated Show resolved Hide resolved
@yzhliu yzhliu added the status: need update need update based on feedbacks label Aug 25, 2019
@comaniac
Copy link
Contributor Author

@wweic thanks for review. Please see my comments and new commits.

@icemelon icemelon merged commit 062f8cc into apache:master Aug 28, 2019
@icemelon
Copy link
Member

Thanks @comaniac @wweic this is now merged

@icemelon icemelon added status: accepted and removed status: need update need update based on feedbacks labels Aug 28, 2019
@comaniac comaniac deleted the autotvm-db-fix branch August 28, 2019 17:05
wweic pushed a commit to wweic/tvm that referenced this pull request Sep 16, 2019
* [AutoTVM] Fix database APIs

* Refactor the byte conversion
wweic pushed a commit to wweic/tvm that referenced this pull request Sep 16, 2019
* [AutoTVM] Fix database APIs

* Refactor the byte conversion
wweic pushed a commit to neo-ai/tvm that referenced this pull request Sep 16, 2019
* [AutoTVM] Fix database APIs

* Refactor the byte conversion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants