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

[Doc] Introduction to module serialization #4564

Merged
merged 1 commit into from
Feb 7, 2020
Merged

Conversation

FrozenGene
Copy link
Member

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

@tqchen

@tqchen
Copy link
Member

tqchen commented Dec 22, 2019

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

@FrozenGene
Copy link
Member Author

FrozenGene commented Dec 26, 2019

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

@tqchen
Copy link
Member

tqchen commented Dec 26, 2019

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

@FrozenGene
Copy link
Member Author

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

Will do.

@FrozenGene
Copy link
Member Author

FrozenGene commented Dec 27, 2019

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
Member

tqchen commented Dec 28, 2019

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

Copy link

@yangjunpro yangjunpro left a comment

Choose a reason for hiding this comment

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

Just some comments.

docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
Copy link
Member

@zhiics zhiics left a comment

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

docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
@FrozenGene
Copy link
Member Author

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

@FrozenGene
Copy link
Member Author

FrozenGene commented Jan 10, 2020

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.

docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
Copy link
Member

@tqchen tqchen left a comment

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

Copy link
Member

@zhiics zhiics left a comment

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.

docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
docs/dev/introduction_to_module_serialization.rst Outdated Show resolved Hide resolved
@tqchen
Copy link
Member

tqchen commented Feb 6, 2020

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

@zhiics
Copy link
Member

zhiics commented Feb 6, 2020

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

@zhiics zhiics merged commit d2cc214 into apache:master Feb 7, 2020
@zhiics
Copy link
Member

zhiics commented Feb 7, 2020

Thanks everyone. This is now merged.

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.

5 participants