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

Support %-based string formatting #45976

Closed
wants to merge 17 commits into from

Conversation

ansley
Copy link

@ansley ansley commented Oct 7, 2020

Stack from ghstack:

Differential Revision: D24374215

@ansley ansley requested a review from apaszke as a code owner October 7, 2020 16:32
ansley pushed a commit that referenced this pull request Oct 7, 2020
ghstack-source-id: f0e3fc4fc230c283047fd5d211f9ac3b8a32f176
Pull Request resolved: #45976
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Oct 7, 2020
@ansley ansley marked this pull request as draft October 7, 2020 16:33
@ansley ansley requested a review from gmagogsfm October 7, 2020 16:33
ansley pushed a commit that referenced this pull request Oct 8, 2020
ghstack-source-id: 23134fdb20f9e6c36ab1c76729debb8f32ed710f
Pull Request resolved: #45976
@ansley ansley marked this pull request as ready for review October 8, 2020 01:49
@ansley ansley requested a review from gmagogsfm October 8, 2020 02:16
Copy link
Contributor

@gmagogsfm gmagogsfm left a comment

Choose a reason for hiding this comment

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

Reviewed part of it, please address current comments and we can iterate.

c10/util/C++17.h Outdated Show resolved Hide resolved
test/test_jit.py Outdated Show resolved Hide resolved
torch/csrc/jit/frontend/ir_emitter.cpp Outdated Show resolved Hide resolved
test/test_jit.py Outdated Show resolved Hide resolved
test/test_jit.py Outdated Show resolved Hide resolved
torch/csrc/jit/runtime/register_prim_ops.cpp Outdated Show resolved Hide resolved
@ansley ansley requested a review from gmagogsfm October 9, 2020 23:39
ansley pushed a commit that referenced this pull request Oct 9, 2020
ghstack-source-id: c6542e392147904c1261f3cd82c65921522c40c6
Pull Request resolved: #45976
@dr-ci
Copy link

dr-ci bot commented Oct 9, 2020

💊 CI failures summary and remediations

As of commit 59b6fd0 (more details on the Dr. CI page):


None of the CI failures appear to be your fault 💚



🚧 5 fixed upstream failures:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

Since your merge base is older than viable/strict, run these commands:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

Check out the recency history of this "viable master" tracking branch.


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.

See how this bot performed.

This comment has been revised 71 times.

c10/util/C++17.h Outdated Show resolved Hide resolved
torch/csrc/jit/frontend/ir_emitter.cpp Outdated Show resolved Hide resolved
torch/csrc/jit/runtime/vararg_functions.cpp Outdated Show resolved Hide resolved
torch/csrc/jit/runtime/vararg_functions.cpp Outdated Show resolved Hide resolved
torch/csrc/jit/runtime/vararg_functions.cpp Outdated Show resolved Hide resolved
std::vector<char> specifiers = {'d', 'i', 'e', 'E', 'f', 'F', 's', 'c'};
auto format = peek(stack, 0, num_inputs).toStringRef();
auto args = last(stack, num_inputs - 1)[0];
auto args_size = !args.isTuple() ? 1 : args.toTuple()->elements().size();
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a TORCH_CHECK to ensure that there is only 1 input when its type is tuple.

Copy link
Author

Choose a reason for hiding this comment

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

Under what circumstances would there not be only one input? I believe that the modulo operator is always parsed into a Tree with a left child and a right child. This means that a test case like the one below would fail before hitting IREmitter at all.

# This test does not raise the correct error! Instead, it raises "cannot call a value of type 'Tuple[str, int]'"
def test_string_interpolation_with_double_tuple(self):
    with self.assertRaisesRegex(RuntimeError, "Argument to format string must be a single str or Tuple"):
        @torch.jit.script
        def fn(arg1: str, arg2: int, arg3: float, arg4: str) -> str:
            return "%s %d %f %s in template" % (arg1, arg2) (arg3, arg4)
        fn("foo", 1, 1.0, "bar")

torch/csrc/jit/runtime/vararg_functions.cpp Outdated Show resolved Hide resolved
torch/csrc/jit/runtime/vararg_functions.cpp Outdated Show resolved Hide resolved
torch/csrc/jit/runtime/vararg_functions.cpp Outdated Show resolved Hide resolved
test/test_jit.py Outdated Show resolved Hide resolved
ansley pushed a commit that referenced this pull request Oct 12, 2020
ghstack-source-id: c45db31e90244ce83cede64109bfcd5360351220
Pull Request resolved: #45976
@ansley ansley requested a review from gmagogsfm October 12, 2020 21:19
torch/csrc/jit/frontend/ir_emitter.cpp Outdated Show resolved Hide resolved
torch/csrc/jit/frontend/ir_emitter.cpp Outdated Show resolved Hide resolved
torch/csrc/jit/frontend/ir_emitter.cpp Show resolved Hide resolved
torch/csrc/jit/runtime/vararg_functions.cpp Outdated Show resolved Hide resolved
ansley pushed a commit that referenced this pull request Oct 13, 2020
ghstack-source-id: f7b8433f048d9cce155ae3efb8d50a895a0f8ee8
Pull Request resolved: #45976
ansley pushed a commit that referenced this pull request Oct 13, 2020
ghstack-source-id: eca98db6201058e5efff72a0d083bb33b3d634f9
Pull Request resolved: #45976
@ansley ansley requested a review from gmagogsfm October 13, 2020 19:03
test/jit/test_string_formatting.py Outdated Show resolved Hide resolved
test/jit/test_string_formatting.py Outdated Show resolved Hide resolved
torch/csrc/jit/frontend/ir_emitter.cpp Outdated Show resolved Hide resolved
torch/csrc/jit/frontend/ir_emitter.cpp Outdated Show resolved Hide resolved
torch/csrc/jit/runtime/register_special_ops.cpp Outdated Show resolved Hide resolved
torch/csrc/jit/runtime/vararg_functions.cpp Outdated Show resolved Hide resolved
torch/csrc/jit/runtime/vararg_functions.cpp Outdated Show resolved Hide resolved
torch/csrc/jit/runtime/vararg_functions.cpp Outdated Show resolved Hide resolved
torch/csrc/jit/runtime/vararg_functions.cpp Outdated Show resolved Hide resolved
torch/csrc/jit/runtime/vararg_functions.cpp Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #45976 into gh/ansleyadelaide/9/base will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@                    Coverage Diff                    @@
##           gh/ansleyadelaide/9/base   #45976   +/-   ##
=========================================================
  Coverage                     68.38%   68.38%           
=========================================================
  Files                           411      411           
  Lines                         53950    53950           
=========================================================
+ Hits                          36893    36895    +2     
+ Misses                        17057    17055    -2     
Impacted Files Coverage Δ
torch/utils/benchmark/utils/common.py 99.31% <0.00%> (+0.68%) ⬆️
torch/testing/_internal/expecttest.py 78.57% <0.00%> (+1.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc7cd97...4f4dd64. Read the comment docs.

ansley pushed a commit that referenced this pull request Oct 14, 2020
ghstack-source-id: 76d8bef50d3e51cd3da887a44821ce5eeef4725c
Pull Request resolved: #45976
@ansley ansley requested a review from gmagogsfm October 14, 2020 21:46
torch/csrc/jit/runtime/vararg_functions.cpp Outdated Show resolved Hide resolved
torch/csrc/jit/runtime/vararg_functions.cpp Outdated Show resolved Hide resolved
test/jit/test_string_formatting.py Show resolved Hide resolved
test/jit/test_string_formatting.py Outdated Show resolved Hide resolved
@ansley ansley linked an issue Oct 15, 2020 that may be closed by this pull request
ansley pushed a commit that referenced this pull request Oct 15, 2020
ghstack-source-id: 86c85502befd882c8bdefff3d8c3a7d8674bc5d7
Pull Request resolved: #45976
ansley pushed a commit that referenced this pull request Oct 16, 2020
ghstack-source-id: f5600a1af6fa36f1e2d6f140c90425b382d6b2f9
Pull Request resolved: #45976
@ansley ansley requested a review from gmagogsfm October 16, 2020 13:21
ansley pushed a commit that referenced this pull request Oct 16, 2020
ghstack-source-id: f2e5df2e7b6fef76bbdd63bab82d8598095a88a3
Pull Request resolved: #45976
ansley pushed a commit that referenced this pull request Oct 19, 2020
ghstack-source-id: c0c94e2582be01a6288d773bf43acf0e56c1780f
Pull Request resolved: #45976
@ansley ansley deleted the gh/ansleyadelaide/9/head branch October 21, 2020 02:10
@facebook-github-bot
Copy link
Contributor

@ansley merged this pull request in fdc5261.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merged oncall: jit Add this issue/PR to JIT oncall triage queue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

%-based string format should be supported in TorchScript
3 participants