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

[REFACTOR] Replace TensorObj and TensorValue with NDArray #4643

Merged
merged 3 commits into from
Jan 11, 2020
Merged

[REFACTOR] Replace TensorObj and TensorValue with NDArray #4643

merged 3 commits into from
Jan 11, 2020

Conversation

zhiics
Copy link
Member

@zhiics zhiics commented Jan 7, 2020 •
edited
Loading

This PR removes the TensorObj in VM and TensorValue in interpreter. Instead, we use NDArray directly since it has been migrated to the unified object protocol, #4599

Update: NodeBase is also replaced by Object in this PR.

@zhiics zhiics force-pushed the tensor_to_ndarray branch 3 times, most recently from 81f9b83 to 1e61991 Compare January 7, 2020 22:10
@zhiics
Copy link
Member Author

zhiics commented Jan 8, 2020

cc @tqchen @wweic @icemelon9 @jroesch

jroesch
jroesch reviewed Jan 8, 2020
View reviewed changes
python/tvm/relay/backend/interpreter.py Outdated

@register_relay_node
class TupleValue(Value):
class TupleValue(NodeBase):
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be Object? or the equivalent Object base class, seems wrong to inherit from node. cc @tqchen

Copy link
Member Author

@zhiics zhiics Jan 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I noticed this as well. It is from here:

https://github.com/apache/incubator-tvm/blob/bc0274d307226408c69226cf922dd916d773e265/python/tvm/_ffi/node.py#L33

and used as the base by Value and RelayNode before so I just kept it.

https://github.com/apache/incubator-tvm/blob/bc0274d307226408c69226cf922dd916d773e265/python/tvm/relay/base.py#L55

Copy link
Member

Choose a reason for hiding this comment

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

There is no Node anymore in the c++ side, see #4603 and #4647

So NodeBase was a legacy item that we might need to upgrade on the python side.

Copy link
Member

Choose a reason for hiding this comment

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

Let us inherit from Object for now, and have a subsequent PR to cleanup the python side.

Also seems we might be able to use Array(or ADT, see discussion here https://discuss.tvm.ai/t/discuss-runtime-array-containers-array-adt-string/4582) for this after @wweic 's PR lands

Copy link
Member Author

@zhiics zhiics Jan 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I tried it, but it looked to me that we have to merge nodebase to object first before I can use object directly as there are fields not implemented in object, like getattr, etc. Should I use nodebase first then we can refactor them together?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just added a commit to replace NodeBase with Object

jroesch
jroesch requested changes Jan 8, 2020
View reviewed changes
Copy link
Member

@jroesch jroesch left a comment

Choose a reason for hiding this comment

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

Mostly looks good, one question about Python wrapper code.

wweic
wweic reviewed Jan 8, 2020
View reviewed changes
Copy link
Contributor

@wweic wweic left a comment

Choose a reason for hiding this comment

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

Thanks! Just a naming suggestion

include/tvm/relay/interpreter.h Show resolved Hide resolved
include/tvm/relay/interpreter.h Show resolved Hide resolved
include/tvm/relay/interpreter.h Show resolved Hide resolved
@zhiics
Copy link
Member Author

zhiics commented Jan 8, 2020

@wweic I am trying to only refactor tensors in the PR. The other ones may need to be either unified between VM and interpreter or changed. We will need another PR for this.

@wweic
Copy link
Contributor

wweic commented Jan 8, 2020

@wweic I am trying to only refactor tensors in the PR. The other ones may need to be either unified between VM and interpreter or changed. We will need another PR for this.

Make senses. I expect it will need more changes.

@zhiics zhiics force-pushed the tensor_to_ndarray branch 4 times, most recently from 6e1610d to 4f47e16 Compare January 9, 2020 00:14
@zhiics
Copy link
Member Author

zhiics commented Jan 9, 2020

@jroesch can you take another look? Should we merge?

@tqchen
Copy link
Member

tqchen commented Jan 9, 2020

@zhiics please do a rebase

@zhiics
Copy link
Member Author

zhiics commented Jan 10, 2020

@tqchen Done.

@tqchen
Copy link
Member

tqchen commented Jan 10, 2020

still need to wait for @jroesch 's approval

@jroesch jroesch merged commit 86092de into apache:master Jan 11, 2020
@zhiics zhiics deleted the tensor_to_ndarray branch January 11, 2020 03:19
@tqchen tqchen mentioned this pull request Jan 13, 2020
Closed
This was referenced Jan 15, 2020
Merged
Merged
alexwong pushed a commit to alexwong/tvm that referenced this pull request Feb 26, 2020
@zhiics @alexwong
[REFACTOR] Replace TensorObj and TensorValue with NDArray (apache#4643)
0f583ee 
* replace TensorObj and TensorValue with NDArray

* NodeBase to Object in Python

* rebase
alexwong pushed a commit to alexwong/tvm that referenced this pull request Feb 28, 2020
@zhiics @alexwong
[REFACTOR] Replace TensorObj and TensorValue with NDArray (apache#4643)
d8fb2cf 
* replace TensorObj and TensorValue with NDArray

* NodeBase to Object in Python

* rebase
zhiics added a commit to neo-ai/tvm that referenced this pull request Mar 2, 2020
@zhiics
[REFACTOR] Replace TensorObj and TensorValue with NDArray (apache#4643)
4ae817a 
* replace TensorObj and TensorValue with NDArray

* NodeBase to Object in Python

* rebase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wweic wweic wweic left review comments

@jroesch jroesch jroesch approved these changes

@tqchen tqchen tqchen approved these changes

Assignees

@jroesch jroesch

Labels
status: accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zhiics @wweic @tqchen @jroesch