Skip to content

[Doc] Introduction to module serialization - #4564

Merged
zhiics merged 1 commit into
apache:masterfrom
FrozenGene:doc
Feb 7, 2020
Merged

[Doc] Introduction to module serialization#4564
zhiics merged 1 commit into
apache:masterfrom
FrozenGene:doc

Conversation

@FrozenGene

Copy link
Copy Markdown
Member

Follow up of #4532, this doc for introducing module serialization and implementation details how we achieve it.

@tqchen

@tqchen

tqchen commented Dec 22, 2019

Copy link
Copy Markdown
Member

Would be great if we can get more readers to help review

Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
@FrozenGene
FrozenGeneforce-pushed the doc branch 2 times, most recently from 678e2c5 to 80e2414CompareDecember 23, 2019 02:35
@FrozenGene

FrozenGene commented Dec 26, 2019

Copy link
Copy Markdown
MemberAuthor

Anyone has interest in helping to review and give feedback? @zhiics@yzhliu@tqchen Thanks.

@tqchen

Copy link
Copy Markdown
Member

It would be great if we can add a figure explaining the storage relation and data structure.

@FrozenGene

Copy link
Copy Markdown
MemberAuthor

It would be great if we can add a figure explaining the storage relation and data structure.

Will do.

@FrozenGene

FrozenGene commented Dec 27, 2019

Copy link
Copy Markdown
MemberAuthor

It would be great if we can add a figure explaining the storage relation and data structure.

Will do.

@tqchen need to double check to make sure I understand correctly.

The doc describes:

  • Module import relationship. i.e. LLVM imports cuda module. <- No need to have figure in my opinion as it only have two modules and is a simple relationship

  • Serialization Format.

binary_blob_size
binary_blob_type_key
binary_blob_logic
...
_import_tree
_import_tree_logic

<- No need to have figure, as this simple table could describe cleanly.

@tqchen

Copy link
Copy Markdown
Member

sounds good, would love to see others' thoughts, @zhiics@yzhliu@ajtulloch@ZihengJiang

@yangjunproyangjunpro left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just some comments.

Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated

@zhiicszhiics left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few high level comments beside the attached grammatical errors and typos.

  • A diagram would be more helpful for ppl to understand. I think the one Tianqi provided in the RFC could be borrowed
  • A little more details about export library can be explained, such as how user provided compilation flags/options can be taken

Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst
Comment threaddocs/dev/introduction_to_module_serialization.rst
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
@FrozenGene

Copy link
Copy Markdown
MemberAuthor

Thanks the comment and suggestion of @zhiics@yzhliu@yangjunpro I will address the comment in the following days.

@FrozenGene

FrozenGene commented Jan 10, 2020

Copy link
Copy Markdown
MemberAuthor

Have addressed the comments. I also add some words of PackImportToC existed in the PR #4657 , so we should be better handle it after that PR in.

Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated

@tqchentqchen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

my part has been address, @zhiics@yzhliu would be great if you can help followup

@zhiicszhiics left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@FrozenGene would be great to have a few more ppl read through the document because I still see a number of typos and broken sentences.

@yangjunpro@yzhliu please do another around when you have cycles.

Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
Comment threaddocs/dev/introduction_to_module_serialization.rst Outdated
@tqchen

Copy link
Copy Markdown
Member

ping @zhiics@yzhliu@FrozenGene@yangjunpro please followup on this thread :)

@zhiics

Copy link
Copy Markdown
Member

LGTM now. I will wait till tomorrow to see if there are any more comments.

@zhiics
zhiics merged commit d2cc214 into apache:masterFeb 7, 2020
@zhiics

Copy link
Copy Markdown
Member

Thanks everyone. This is now merged.

anijain2305 pushed a commit to anijain2305/tvm that referenced this pull request Feb 10, 2020
alexwong pushed a commit to alexwong/tvm that referenced this pull request Feb 26, 2020
alexwong pushed a commit to alexwong/tvm that referenced this pull request Feb 28, 2020
zhiics pushed a commit to neo-ai/tvm that referenced this pull request Mar 2, 2020
Sign up for freeto 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.

5 participants

@FrozenGene@tqchen@zhiics@yangjunpro@yzhliu