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

[RFC] Meta-data default behavior Relay astext API #2461

Closed
tqchen opened this issue Jan 18, 2019 · 7 comments
Closed

[RFC] Meta-data default behavior Relay astext API #2461

tqchen opened this issue Jan 18, 2019 · 7 comments

Comments

@tqchen
Copy link
Member

tqchen commented Jan 18, 2019

Background

We introduced the API relay_node.astext to dump relay code into text format. This can be useful for debugging purposes, so we can get text format out. In order to enable round-trip of between the text format and the in-memory one, we introduced a meta-data section to the text format. The metadata section can be used to store the content of the model parameters as relay.Constant in JSON format and is appended to the end of the text format.

Problem

The question is: should we set show_meta_data option in as text by default?

  • currently it is set to True

  • When we use astext for debugging, we want to skip printing meta_data section, because the base64 content of constants are meaningless and can be explosive

  • When we use astext for serialization, we need to include meta_data, otherwise we cannot recover the data.

Possible Solutions

  1. set default of show_meta_data to be False, and introduce perhaps new API such as save_xx for serialization.

  2. keep the current API default(always print meta-data), but overload __str__ and ___repr__ of RelayNode to use astext. So we can use astext for serialziation, but use print directly for debug.

Please share your thoughts, cc @dmlc/tvm-comitter @jroesch

@zhiics
Copy link
Member

zhiics commented Jan 18, 2019

I think it is probably worthy to have a separate API for serialization. Overloading can suppress meta in python, but we will still print it in c++ during debugging if False is not given.

@tqchen
Copy link
Member Author

tqchen commented Jan 18, 2019

To unify the API, if we take the other way, astext defaults to show meta-data.

We can have also two functions in C++, AsText, which default to show meta-data, and Print, which by default do not show meta-data. We can also directly overload operator<< for the c++ side.

@zhiics
Copy link
Member

zhiics commented Jan 18, 2019

Yes, then I don't have any preference. But sol. 2 looks cleaner.

@tqchen
Copy link
Member Author

tqchen commented Jan 20, 2019

@jroesch what are your thoughts on this?

@ZihengJiang
Copy link
Contributor

ZihengJiang commented Jan 25, 2019

Prefer solution 2. When I try to debug the IR, I will try print(func) intuitively, but the information does not help a lot. So overloading __str__ sounds a good idea

@tqchen
Copy link
Member Author

tqchen commented Jan 25, 2019

OK, seems everyone is leaning towards solution 2, contribution is welcomed

@tqchen
Copy link
Member Author

tqchen commented Feb 3, 2019

#2533

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

No branches or pull requests

3 participants