Generalize constant_table from tensor only to ivalue #37631
Conversation
facebook-github-bot
added
the
oncall: jit
💊 CI failures summary and remediationsAs of commit 8d3208b (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 61 times. |
bzinodev
force-pushed
the
constant_table
branch
2 times, most recently
from
61042dd
to
29540c3
Compare
There was a problem hiding this comment.
@bzinodev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
bzinodev
force-pushed
the
constant_table
branch
2 times, most recently
from
cff65dd
to
7839272
Compare
bzinodev
force-pushed
the
constant_table
branch
3 times, most recently
from
8239218
to
a02c73a
Compare
There was a problem hiding this comment.
@bzinodev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@bzinodev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@bzinodev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@bzinodev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| // Apply visitor to every subvalue. | ||
| // TODO: There are several places that recurse over IValue. This is fragile. | ||
| // This visitor should be used to recurse over ivalues. | ||
| void visit(const std::function<bool (const IValue &)>& visitor) const; |
There was a problem hiding this comment.
Can you add a short comment explaining how the return bool is used?
There was a problem hiding this comment.
@bzinodev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| @@ -101,6 +101,46 @@ TypePtr IValue::type() const { | |||
| TORCH_INTERNAL_ASSERT(false, "unhandled case in IValue::type()"); | |||
| } | |||
|
|
|||
| void IValue::visit(const std::function<bool (const IValue &)>& visitor) const { | |||
There was a problem hiding this comment.
I recommend against adding this to IValue: it does not contain tests, and is not even turned on in this PR. It is the kind of simple function that people looking at ivalue will think it tested and implemented correctly. This should be implemented locally where needed unless we commit to completely implementing it and testing it.
| void printConstant(TaggedStringStream& stmt, const IValue& v) { | ||
| const auto customFormatter = [&](std::ostream& ss, const IValue& v) { | ||
| if (v.isTensor()) { | ||
| ss << "CONSTANTS.c" << getOrAddTensorConstant(v.toTensor()); | ||
| if (v.isTensor() || containsNonASCIIString(v)) { |
There was a problem hiding this comment.
I feel like this isn't the right condition: we should be able to correctly print non-ascii strings with the appropriate escapes. In fact, I think we actually do. I think what is going on here is that some aggregate constant is getting printed through a call to its pretty printer, and the bug lies in the fact that the pretty printer for that aggregate doesn't recursively invoke our our repr code for strings. I feel like this bug should be fixed there, this feels like a workaround.
There was a problem hiding this comment.
IMO, only small string should be allowed to be inlined. All large constant (lists,dicts, strings) should be in the constant table. I followed this logic and time is spent parsing string literals. I would not spend time adding logic to parse unicode strings.
There was a problem hiding this comment.
@bzinodev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Currently only constant except tensor must be inlined during serialization. Tensor are stored in the contant table. This patch generalizes this capability to any IValue. This is particularly useful for non ASCII string literal that cannot be inlined.
There was a problem hiding this comment.
@bzinodev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@bzinodev has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Currently JIT serialization allows only tensor in the constant table (CONSTANTS section). All other constants are inlined. In some cases these constant can be long and contains non ASCII strings, currently not serialized.