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

Add string versions of argument funcs in jit Node #45464

Closed
wants to merge 1 commit into from

Conversation

spaugh
Copy link
Contributor

@spaugh spaugh commented Sep 28, 2020

Summary:
Usage of Symbols to find arguments requires one to generate a nonsense symbol for inputs which don't already have one. The intention of symbols appears to be something of an internalized string, but the namespace component doesn't apply to an argument. In order to access the arguments by name without adding new symbols, versions of those functions with std::string input was added. These can be proved valid based on the existing codepath. Additionally, a hasNamedInput convenience function was added to remove the necessity of a try/catch block in user code.

The primary motivation is to be able to easily handle the variable number of arguments in glow, so that the arange op may be implemented.

Differential Revision: D23972315

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23972315

@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Sep 28, 2020
@dr-ci
Copy link

dr-ci bot commented Sep 28, 2020

💊 CI failures summary and remediations

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


Commit c2146b2 was recently pushed. Waiting for builds...


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

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23972315

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D23972315

Summary:
Pull Request resolved: pytorch#45464

Usage of Symbols to find arguments requires one to generate a nonsense symbol for inputs which don't already have one. The intention of symbols appears to be something of an internalized string, but the namespace component doesn't apply to an argument. In order to access the arguments by name without adding new symbols, versions of those functions with std::string input was added. These can be proved valid based on the existing codepath. Additionally, a hasNamedInput convenience function was added to remove the necessity of a try/catch block in user code.

The primary motivation is to be able to easily handle the variable number of arguments in glow, so that the arange op may be implemented.

Differential Revision: D23972315

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

This pull request was exported from Phabricator. Differential Revision: D23972315

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. Current findArgument api internally converts the symbol to a string anyway, and this is a nicer api. https://github.com/pytorch/pytorch/blob/master/torch/csrc/jit/ir/ir.cpp#L850

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in cdf93b0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fb-exported 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.

None yet

4 participants