Skip to content

Conversation

driazati
Copy link
Contributor

@driazati driazati commented Sep 22, 2018

PR adds support for int and float default values for script functions (values get cast to Tensor implicitly). Resolves expressions via eval (but only for types that can convert to a Tensor)

@apaszke @zdevito @ezyang

Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
};

////////////////////////////////////////////////////////////////////////////////
// Helper nodes (mostly for function arguments)

This comment was marked as off-topic.

@ailzhang ailzhang added the oncall: jit Add this issue/PR to JIT oncall triage queue label Sep 25, 2018
David Riazati added 3 commits September 27, 2018 14:43
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
Copy link
Contributor

@zdevito zdevito left a comment

Choose a reason for hiding this comment

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

Adding support for default arguments is a great thing to work on, but I think this is ultimately not the right approach. Instead, I would suggest a much simpler set of changes:

  1. Extract the default values from the original functions using: https://stackoverflow.com/questions/12627118/get-a-function-arguments-default-value
  2. Compile the function into a graph as is, without changing the compiler or ir.h, deriving a script::Method object without any default values in it. Use the types inside of the derived FunctionSchemaobject and inline IValue toIValue(py::handle obj, const TypePtr& type) to turn the python default value into an IValue.
  3. Generate a new FunctionSchema object that includes the default IValues, and call setSchema on the Method to install it.

This approach is more modular (it doesn't require changes to AST parsing, the compiler, or the IR), and it relies on components we already have to robustly convert python values into IValue objects.

struct Value {
TH_DISALLOW_COPY_AND_ASSIGN(Value);
Value(Node * node_, size_t offset_);
virtual ~Value() {}

This comment was marked as off-topic.

at::optional<IValue> default_value = at::nullopt;
if (decl_arg.has_default()) {
// If the Param has a default value, convert it to an IValue
Const default_const = decl_arg.default_value();

This comment was marked as off-topic.

@driazati driazati closed this Oct 4, 2018
facebook-github-bot pushed a commit that referenced this pull request Oct 12, 2018
Summary:
Add support for default values on script functions and Modules

Followup to #11962
Pull Request resolved: #12345

Reviewed By: michaelsuo

Differential Revision: D10263613

Pulled By: driazati

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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants