Skip to content

Improve sugared value's error message #42889

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
gmagogsfm wants to merge 1 commit into from
Closed

Improve sugared value's error message #42889

gmagogsfm wants to merge 1 commit into from

Conversation

@gmagogsfm
Copy link
Contributor

@gmagogsfm gmagogsfm commented Aug 12, 2020 •
edited
Loading

Stack from ghstack:

I think most (if not all) cases where this code path is reached can be attributed to closing over a global variable.
Improving error message to make this clearer to users.

close #41288

@gmagogsfm gmagogsfm requested a review from apaszke as a code owner August 12, 2020 00:30
gmagogsfm added a commit that referenced this pull request Aug 12, 2020
@gmagogsfm
Improve sugared value's error message
d337eb5 
I think most (if not all) cases where this code path is reached can be attributed to closing over a global variable.
Improving error message to make this clearer to users.

ghstack-source-id: 4f89237
Pull Request resolved: #42889
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Aug 12, 2020
@gmagogsfm gmagogsfm requested a review from suo August 12, 2020 00:33
@dr-ci
Copy link

dr-ci bot commented Aug 12, 2020 •
edited
Loading

💊 CI failures summary and remediations

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

  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-CircleCI failure(s)

Extra GitHub checks: 1 failed

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 28 times.

gmagogsfm added a commit that referenced this pull request Aug 18, 2020
@gmagogsfm
Improve sugared value's error message
92ec505 
I think most (if not all) cases where this code path is reached can be attributed to closing over a global variable.
Improving error message to make this clearer to users.

ghstack-source-id: e8817cc
Pull Request resolved: #42889
@gmagogsfm gmagogsfm linked an issue Aug 18, 2020 that may be closed by this pull request
python value of type 'str' cannot be used as a value #41288
Closed
@gmagogsfm gmagogsfm requested a review from eellison August 21, 2020 19:22
eellison
eellison approved these changes Sep 2, 2020
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.

Looks good pending move to more specific SugaredValue PythonValue

torch/csrc/jit/frontend/sugared_value.h
// what can we do with this thing?
// use it as a value e.g. `this + 4`
virtual Value* asValue(const SourceRange& loc, Function& m) {
throw ErrorReport(loc) << kind() << " cannot be used as a value";
Copy link
Contributor

@eellison eellison Sep 2, 2020

Choose a reason for hiding this comment

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

Can you move this to PythonValue (which is the specific closed over value).

pytorch/torch/csrc/jit/python/python_sugared_value.h

Line 32 in 35a36c1

struct VISIBILITY_HIDDEN PythonValue : public SugaredValue {

Copy link
Contributor Author

@gmagogsfm gmagogsfm Sep 18, 2020

Choose a reason for hiding this comment

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

Done.

@gmagogsfm gmagogsfm changed the base branch from gh/gmagogsfm/4/base to master September 18, 2020 06:14
facebook-github-bot
facebook-github-bot reviewed Sep 18, 2020
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.

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

@codecov
Copy link

codecov bot commented Sep 18, 2020 •
edited
Loading

Codecov Report

Merging #42889 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #42889   +/-   ##
=======================================
  Coverage   67.91%   67.91%           
=======================================
  Files         384      384           
  Lines       49839    49839           
=======================================
+ Hits        33847    33849    +2     
+ Misses      15992    15990    -2
Impacted Files Coverage Δ
.../testing/_internal/distributed/distributed_test.py 31.06% <0.00%> (+0.05%) ⬆️
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 a383517...e302a92. Read the comment docs.

@lw lw removed request for lw and zhaojuanmao September 18, 2020 10:10
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 18, 2020

@gmagogsfm merged this pull request in 174cbff.

@facebook-github-bot facebook-github-bot deleted the gh/gmagogsfm/4/head branch September 22, 2020 14:18
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

@suo suo Awaiting requested review from suo

@albanD albanD Awaiting requested review from albanD

@beauby beauby Awaiting requested review from beauby

@ebetica ebetica Awaiting requested review from ebetica

@ezyang ezyang Awaiting requested review from ezyang

@fmassa fmassa Awaiting requested review from fmassa

@goldsborough goldsborough Awaiting requested review from goldsborough

@jiayisuse jiayisuse Awaiting requested review from jiayisuse

@mrshenli mrshenli Awaiting requested review from mrshenli

@osalpekar osalpekar Awaiting requested review from osalpekar

@pietern pietern Awaiting requested review from pietern

@pritamdamania87 pritamdamania87 Awaiting requested review from pritamdamania87

@soumith soumith Awaiting requested review from soumith

@yf225 yf225 Awaiting requested review from yf225

Assignees

No one assigned

Labels

Merged oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

python value of type 'str' cannot be used as a value

5 participants

@gmagogsfm @facebook-github-bot @eellison @mruberry