Skip to content

make default string arguments in schemas human readable #27088

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.

Sign up for GitHub

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

Jump to bottom
Closed
free25zer wants to merge 5 commits into from
Closed

make default string arguments in schemas human readable #27088

free25zer wants to merge 5 commits into from

Conversation

@free25zer
Copy link
Contributor

@free25zer free25zer commented Sep 30, 2019

[jit] String default args get printed as ascii values #25804
#25804

@free25zer free25zer requested a review from apaszke as a code owner September 30, 2019 20:03
@pytorchbot pytorchbot added oncall: jit Add this issue/PR to JIT oncall triage queue module: internals Related to internal abstractions in c10 and ATen module: pybind Related to our Python bindings / interactions with other Python libraries labels Sep 30, 2019
@Krovatkin Krovatkin requested a review from eellison September 30, 2019 20:10
@Krovatkin
Copy link
Contributor

Krovatkin commented Sep 30, 2019 •
edited
Loading

@eellison could you please take a look when you have a mnt? Seems to be a reasonable solution and you seem to have dealt with printQuotedString in the past.

eellison
eellison approved these changes Sep 30, 2019
View reviewed changes
Copy link
Contributor

@eellison eellison left a comment

Choose a reason for hiding this comment

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

LGTM. Although this change seems benign to me a test would be nice

facebook-github-bot
facebook-github-bot reviewed Oct 1, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

eellison
eellison reviewed Oct 1, 2019
View reviewed changes
test/test_jit.py
self.bar = y # can't assign to non-initialized attr

def test_schema_human_readable(self):
with self.assertRaisesRegex(RuntimeError, "nearest"):
Copy link
Contributor

@eellison eellison Oct 1, 2019

Choose a reason for hiding this comment

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

Why are we matching against "nearest" ?

Copy link
Contributor Author

@free25zer free25zer Oct 1, 2019

Choose a reason for hiding this comment

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

In the original issue it was the argument to mode that was printed out as octal:
mode='\156\145\141\162\145\163\164'
When it should just be
mode='nearest'
So I am matching against nearest to make sure that we are printing it correctly

Copy link
Contributor

@eellison eellison Oct 1, 2019

Choose a reason for hiding this comment

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

makes sense, thanks

Copy link
Contributor Author

@free25zer free25zer Oct 1, 2019

Choose a reason for hiding this comment

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

I'll add in a comment in the test in a PR

@eellison
Copy link
Contributor

eellison commented Oct 1, 2019

@pytorchbot merge this please

facebook-github-bot
facebook-github-bot reviewed Oct 2, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

zdevito pushed a commit to zdevito/ATen that referenced this pull request Oct 2, 2019
@free25zer @facebook-github-bot
make default string arguments in schemas human readable (#27088)
b7b9b5f 
Summary:
[jit] String default args get printed as ascii values pytorch/pytorch#25804
pytorch/pytorch#25804
Pull Request resolved: pytorch/pytorch#27088

Differential Revision: D17689732

Pulled By: Krovatkin

fbshipit-source-id: f385b2fe44c5a2387bfcb6484edf7faa92bc8edf
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Oct 2, 2019

@Krovatkin merged this pull request in bb51980.

pdlive215 pushed a commit to pdlive215/pytorch that referenced this pull request Nov 27, 2019
@free25zer @pdlive215
make default string arguments in schemas human readable (pytorch#27088)
b60238d 
Summary:
[jit] String default args get printed as ascii values pytorch#25804
pytorch#25804
Pull Request resolved: pytorch#27088

Differential Revision: D17689732

Pulled By: Krovatkin

fbshipit-source-id: f385b2fe44c5a2387bfcb6484edf7faa92bc8edf
thiagocrepaldi pushed a commit to thiagocrepaldi/pytorch that referenced this pull request Feb 4, 2020
@free25zer
make default string arguments in schemas human readable (pytorch#27088)
ed445db 
Summary:
[jit] String default args get printed as ascii values pytorch#25804
pytorch#25804
Pull Request resolved: pytorch#27088

Differential Revision: D17689732

Pulled By: Krovatkin

fbshipit-source-id: f385b2fe44c5a2387bfcb6484edf7faa92bc8edf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@eellison eellison eellison approved these changes

@apaszke apaszke Awaiting requested review from apaszke

Assignees

No one assigned

Labels

Merged module: internals Related to internal abstractions in c10 and ATen module: pybind Related to our Python bindings / interactions with other Python libraries oncall: jit Add this issue/PR to JIT oncall triage queue open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@free25zer @Krovatkin @eellison @facebook-github-bot @ezyang @pytorchbot @mruberry