Skip to content

[jit] Add self to Python printer reserved words #15318

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

Closed
wants to merge 8 commits into from

Conversation

driazati
Copy link
Contributor

@driazati driazati commented Dec 17, 2018

This adds self to the list of reserved words and also sorts the lines and prevents the tracer from naming values 'self' (which happens in torch/tensor.py)

Fixes #15240

This adds `self` to the list of reserved words and also sorts the lines
@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Dec 17, 2018
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.

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

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.

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

@@ -142,19 +142,17 @@ void createTensorToParameterNameMap(
// they are keywords or namespaces used in the output
const static std::unordered_set<std::string> reserved_names = {

Choose a reason for hiding this comment

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

What about new keywords like async?

Copy link

@sidazhang sidazhang Dec 19, 2018

Choose a reason for hiding this comment

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

Maybe follow this list?

https://github.com/python/cpython/blob/3.7/Lib/keyword.py

We are also missing del here

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.

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

Copy link
Collaborator

@jamesr66a jamesr66a left a comment

Choose a reason for hiding this comment

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

LGTM. Not sure I'm a fan of discarding the name while tracing, but any other solutions would probably be more involved

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.

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

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.

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

@@ -223,10 +223,10 @@ def _get_interpreter_name_for_var(var):

for k, v in f_locals.items():
if isinstance(v, torch.Tensor) and var is v:
return k
return k if k != 'self' else ''
Copy link
Contributor

Choose a reason for hiding this comment

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

A comment for why this is unwanted would be very useful.

@suo suo added this to the 1.0.1 milestone Jan 10, 2019
suo pushed a commit that referenced this pull request Jan 18, 2019
Summary:
This adds `self` to the list of reserved words and also sorts the lines and prevents the tracer from naming values 'self' (which happens in torch/tensor.py)

Fixes #15240
Pull Request resolved: #15318

Differential Revision: D13540192

Pulled By: driazati

fbshipit-source-id: 46ae02e51b1b31d5c62110fa83ba258ea6bada27
soumith pushed a commit that referenced this pull request Jan 19, 2019
Summary:
This adds `self` to the list of reserved words and also sorts the lines and prevents the tracer from naming values 'self' (which happens in torch/tensor.py)

Fixes #15240
Pull Request resolved: #15318

Differential Revision: D13540192

Pulled By: driazati

fbshipit-source-id: 46ae02e51b1b31d5c62110fa83ba258ea6bada27
@soumith soumith added the cherry-picked This PR was cherry-picked onto a release branch from master label Jan 19, 2019
soumith pushed a commit that referenced this pull request Jan 29, 2019
Summary:
This adds `self` to the list of reserved words and also sorts the lines and prevents the tracer from naming values 'self' (which happens in torch/tensor.py)

Fixes #15240
Pull Request resolved: #15318

Differential Revision: D13540192

Pulled By: driazati

fbshipit-source-id: 46ae02e51b1b31d5c62110fa83ba258ea6bada27
@ezyang ezyang added the merged label Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked This PR was cherry-picked onto a release branch from master oncall: jit Add this issue/PR to JIT oncall triage queue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[JIT] LSTM and LSTMCell
9 participants