Strings lexing, parsing, implementation in print by eellison · Pull Request #9324 · pytorch/pytorch

eellison · 2018-07-10T22:24:29Z

This PR adds strings to the ast and implements them for print statements. Strings are lifted as attributes to the print node. They must be arguments to print itself, not as an argument for an object that is passed to print. If they are encountered elsewhere a NYI exception will be thrown.

test/test_jit.py

torch/csrc/jit/register_prim_ops.cpp

          size_t num_inputs = node->inputs().size();
-          return [num_inputs](Stack& stack) {
+          std::vector<std::string> strings = node->ss(Symbol::attr("strings"));
+          std::vector<int64_t> indices = node->is(Symbol::attr("string_indices"));


torch/csrc/jit/register_prim_ops.cpp

-                std::cout << at::Tensor(tensor_impl, true);
-              } else if (!i.defined()) {
-                std::cout << "<undefined tensor>";
+              if (str_i < strings.size() && int(ten_i + str_i) == indices[str_i]) {


torch/csrc/jit/script/compiler.cpp

        return emitTernaryIf(TernaryIf(tree));
      } break;
+      case TK_STRINGLITERAL: {  
+        throw std::runtime_error("NYI: string literals are only supported as an argument to print\n");


torch/csrc/jit/script/compiler.cpp

+      }
+    }
+    auto filtered_inputs = getNamedValues(trees_, true, identity);
+    ensureTensors(ident.range(), toValues(filtered_inputs));


torch/jit/frontend.py

+    @staticmethod
+    def build_Str(ctx, expr):
+        value = str(expr.s)
+        r = ctx.make_range(expr.lineno, expr.col_offset, expr.col_offset + len(value))


torch/csrc/jit/script/lexer.h

+      return false;
+    int quote_len = isCharCount(quote, str, start, 3) ? 3 : 1; 
+    size_t end = start + quote_len; 
+    while(end < str.size() && !isCharCount(quote, str, end, quote_len)) {


torch/csrc/jit/script/lexer.h

+      if (str[end] == '\n' && quote_len != 3) {
+        return false;
+      }
+      if (str[end] == '\\') {


torch/csrc/jit/script/parser.h

+    return start + len <= str.size() && std::count(str.begin() + start, str.begin() + start + len, c) == len; 
+  }
+
+  std::string parseLexedString(const std::string &str, size_t start) {


ezyang · 2018-07-11T16:52:51Z

@pytorchbot retest this please

facebook-github-bot

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

zdevito

Looking good -- I have a bunch of organizational suggestions to make the code easier to understand and maintain.

torch/csrc/jit/register_prim_ops.cpp

        [](Node* node) {
          size_t num_inputs = node->inputs().size();
-          return [num_inputs](Stack& stack) {
+          std::vector<std::string> strings = node->ss(Symbol::attr("strings"));


torch/csrc/jit/script/compiler.cpp

+      }
+    }
+    auto filtered_inputs = getNamedValues(trees_, true, identity);
+    ensureTensors(ident.range(), toValues(filtered_inputs));


torch/csrc/jit/script/compiler.cpp

      }
      case TK_APPLY: {
        auto apply = Apply(tree);
+        Ident ident = Var(apply.callee()).name();


torch/csrc/jit/script/compiler.cpp

        return emitTernaryIf(TernaryIf(tree));
      } break;
+      case TK_STRINGLITERAL: {  
+        throw std::runtime_error("NYI: string literals are only supported as an argument to print\n");


torch/csrc/jit/script/compiler.cpp

        return emitTernaryIf(TernaryIf(tree));
      } break;
+      case TK_STRINGLITERAL: {  
+        throw std::runtime_error("NYI: string literals are only supported as an argument to print\n");


torch/csrc/jit/script/lexer.h

+      if (str[end] == '\n' && quote_len != 3) {
+        return false;
+      }
+      if (str[end] == '\\') {


torch/csrc/jit/script/parser.h

+    return start + len <= str.size() && std::count(str.begin() + start, str.begin() + start + len, c) == len; 
+  }
+
+  std::string parseLexedString(const std::string &str, size_t start) {


torch/csrc/jit/script/parser.h

+    size_t next_start = end + quote_len; 
+    //find start of next string
+    while(next_start < str.size() && str[next_start] != '\'' && str[next_start] != '\"') {
+      if (str[next_start] == '#') {


torch/csrc/jit/script/parser.h

+    char quote = str[start];
+    int quote_len = isCharCount(quote, str, start, 3) ? 3 : 1;
+    size_t end = start + quote_len;
+    while(!isCharCount(quote, str, end, quote_len)) {


torch/csrc/jit/script/parser.h

+        next_start = str.find('\n', next_start);
+      }
+      next_start++;
+    }


eellison · 2018-07-11T19:30:32Z

Thanks adam and zach for the thorough review!!

ezyang · 2018-07-14T16:33:59Z

@eellison I'll let you import this when you're ready

apaszke

Haven't checked the lexer/parser. Generally looks ok, but we'll probably want to revise our strategy for handling string literals. I think it would be much cleaner to treat them as first class values in the compiler, and then have a string lowering pass that would eliminate them from the interpreter, and embed them into prim::Print nodes. We shouldn't really have any special casing here.

Also, this will need to be rebased on top of #9584 once it lands. It's a much bigger and more complicated PR, and it cleans up parts of the code you touch.

test/expect/TestScript.test_string_cu.expect

torch/csrc/jit/register_prim_ops.cpp

torch/jit/frontend.py

+    @staticmethod
+    def build_Str(ctx, expr):
+        value = str(expr.s)
+        r = ctx.make_range(expr.lineno, expr.col_offset, expr.col_offset + len(value))


torch/csrc/jit/script/compiler.cpp

Changes addressed

facebook-github-bot

eellison has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

eellison · 2018-08-02T15:31:50Z

@pytorchbot retest pr/caffe2-py2-gcc4.8-ubuntu14.04-test please

eellison · 2018-08-02T15:33:24Z

@pytorchbot retest this please

Summary: This PR adds strings to the ast and implements them for print statements. Strings are lifted as attributes to the print node. They must be arguments to print itself, not as an argument for an object that is passed to print. If they are encountered elsewhere a NYI exception will be thrown. Pull Request resolved: pytorch#9324 Reviewed By: jramseyer Differential Revision: D8807128 Pulled By: eellison fbshipit-source-id: 984401ff458ed18d473c6d1bd86750e56c77d078

eellison requested review from apaszke, colesbury, ezyang, gchanan, soumith and zdevito as code owners July 10, 2018 22:24

eellison force-pushed the string_printing branch from b4e7cbd to b2decb1 Compare July 10, 2018 23:11

apaszke reviewed Jul 11, 2018

View reviewed changes

facebook-github-bot reviewed Jul 11, 2018

View reviewed changes

zdevito reviewed Jul 11, 2018

View reviewed changes

eellison requested review from Yangqing, anderspapitto, bddppq, dzhulgakov, ebetica, goldsborough, houseroad, jamesr66a, pietern, smessmer, ssnl, teng-li and zou3519 as code owners July 12, 2018 17:03

eellison force-pushed the string_printing branch from ab23de7 to b2decb1 Compare July 12, 2018 17:10

apaszke reviewed Jul 23, 2018

View reviewed changes

zou3519 added the oncall: jit Add this issue/PR to JIT oncall triage queue label Jul 23, 2018

eellison force-pushed the string_printing branch from 7389ef6 to 928abfd Compare July 25, 2018 23:58

eellison force-pushed the string_printing branch from 256eb20 to 76bd3eb Compare July 31, 2018 01:11

Elias Ellison added 7 commits July 31, 2018 16:20

String lexing, parsing, + print

0ba4cd8

Addressing PR Requests

cdf4bf4

ivalue rebase changes

80dea18

addressing PR comments

5cadf44

Responding to latest round of comments

5f85cdd

Make ConstantString hold pointer to std::string for memory safety

17fc20b

std::string not pointer

ea94ace

eellison force-pushed the string_printing branch 9 times, most recently from 11c1f26 to 4cdb804 Compare August 1, 2018 01:05

zdevito reviewed Aug 1, 2018

View reviewed changes

torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

eellison force-pushed the string_printing branch from 4cdb804 to 8c77c03 Compare August 1, 2018 17:48

make tuples work in python 2, and some small merge fixes

e905658

eellison force-pushed the string_printing branch from 8c77c03 to e905658 Compare August 1, 2018 17:50

facebook-github-bot reviewed Aug 1, 2018

View reviewed changes

facebook-github-bot closed this in 170d297 Aug 2, 2018

wanchaol mentioned this pull request Aug 19, 2018

[JIT] Str aren't supported in print #8751

Closed

ezyang added the merged label Jun 26, 2019

Conversation

eellison commented Jul 10, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

ezyang commented Jul 11, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

zdevito left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

eellison commented Jul 11, 2018

Uh oh!

ezyang commented Jul 14, 2018

Uh oh!

apaszke left a comment

Choose a reason for hiding this comment

Uh oh!