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.

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

[ExecutionEngine] Create runctx and execute(ctx) functions #1907

Merged
merged 1 commit into from
Oct 24, 2018
Merged

[ExecutionEngine] Create runctx and execute(ctx) functions #1907

merged 1 commit into from
Oct 24, 2018

Conversation

gcatron
Copy link
Contributor

@gcatron gcatron commented Oct 22, 2018 •
edited
Loading

Description: Create runctx and execute(ctx) functions for moving placeholder memory allocation from compile time to run time and update tests.
Testing: Ninja all
Documentation: N/A
Progress on #1904 This would bring us to completion of the second step.

rdzhabarov
rdzhabarov reviewed Oct 23, 2018
View reviewed changes
include/glow/ExecutionEngine/ExecutionEngine.h Outdated
@@ -83,6 +83,8 @@ class ExecutionEngine final {

/// Runs a single execution of the function.
void run();
/// Context aware single execution of the function
void runctx(Context &ctx);

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

rdzhabarov
rdzhabarov reviewed Oct 23, 2018
View reviewed changes
Copy link
Contributor

@rdzhabarov rdzhabarov left a comment

Choose a reason for hiding this comment

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

LGTM! Few nits.

include/glow/Backends/CompiledFunction.h Outdated
@@ -16,6 +16,8 @@
#ifndef GLOW_BACKENDS_COMPILEDFUNCTION_H
#define GLOW_BACKENDS_COMPILEDFUNCTION_H

#include "glow/Graph/Context.h"

#include <unordered_map>

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

include/glow/ExecutionEngine/ExecutionEngine.h Outdated
@@ -83,6 +83,8 @@ class ExecutionEngine final {

/// Runs a single execution of the function.
void run();
/// Context aware single execution of the function

This comment was marked as off-topic.

Sign in to view

include/glow/Backends/CompiledFunction.h
@@ -28,6 +30,7 @@ class CompiledFunction {

/// Execute the network.
virtual void execute() = 0;
virtual void execute(Context &ctx) = 0;

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

nadavrot
nadavrot requested changes Oct 23, 2018
View reviewed changes
Copy link
Contributor

@nadavrot nadavrot left a comment

Choose a reason for hiding this comment

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

@gcatron I think that it would be better to change run to run(ctx). Later, when we finish the migration we can remove compile(ctx).

Also, have you emailed the partners mailing list to let them know about this change?

nadavrot
nadavrot reviewed Oct 23, 2018
View reviewed changes
include/glow/ExecutionEngine/ExecutionEngine.h Outdated
@@ -83,6 +83,8 @@ class ExecutionEngine final {

/// Runs a single execution of the function.
void run();

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

lib/Backends/Interpreter/InterpreterFunction.h Outdated
@@ -55,6 +55,8 @@ class InterpreterFunction final : public CompiledFunction {
///@{
~InterpreterFunction() override;

void execute(Context &ctx) override;

void execute() override;

This comment was marked as off-topic.

Sign in to view

qcolombet
qcolombet reviewed Oct 24, 2018
View reviewed changes
include/glow/Backends/CompiledFunction.h
@@ -28,6 +30,7 @@ class CompiledFunction {

/// Execute the network.
virtual void execute() = 0;
virtual void execute(Context &ctx) = 0;

This comment was marked as off-topic.

Sign in to view

include/glow/Backends/CompiledFunction.h Outdated
@@ -28,6 +30,9 @@ class CompiledFunction {

/// Execute the network.
virtual void execute() = 0;
/// Execute the network and allocate Placeholder memory with given
/// Context providing mapping between Placeholder and populated tensor.

This comment was marked as off-topic.

Sign in to view

@nadavrot
Copy link
Contributor

@gcatron Looks good to me. Please make sure to squash the commits.

rdzhabarov
rdzhabarov approved these changes Oct 24, 2018
View reviewed changes
Copy link
Contributor

@rdzhabarov rdzhabarov left a comment

Choose a reason for hiding this comment

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

cool!

@rdzhabarov
Copy link
Contributor

Please, make sure to resolve conflicts and good to go.

re squashing, you could also select an option to "squash and merge" on this PR, up to yous

@gcatron
Added ctx parameter to run and execute.
5ebbbfc 
Updated tests to use new function signatures
@gcatron gcatron force-pushed the add_context_to_ee_run branch from ad6c05f to 5ebbbfc Compare October 24, 2018 20:55
@gcatron gcatron merged commit 366aba3 into pytorch:master Oct 24, 2018
@gcatron gcatron deleted the add_context_to_ee_run branch October 24, 2018 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qcolombet qcolombet qcolombet left review comments

@rdzhabarov rdzhabarov rdzhabarov approved these changes

@nadavrot nadavrot nadavrot approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gcatron @nadavrot @rdzhabarov @qcolombet @facebook-github-bot