Skip to content
This repository has been archived by the owner on Jan 3, 2023. It is now read-only.

Add PlaidML backend #1888

Merged
merged 6 commits into from Oct 29, 2018
Merged

Add PlaidML backend #1888

merged 6 commits into from Oct 29, 2018

Conversation

earhart
Copy link
Contributor

@earhart earhart commented Oct 22, 2018

No description provided.

src/ngraph/runtime/plaidml/plaidml_backend.hpp Outdated Show resolved Hide resolved
src/ngraph/runtime/plaidml/plaidml_backend.cpp Outdated Show resolved Hide resolved
*******************************************************************************/

#include "ngraph/runtime/plaidml/plaidml_builder.hpp"

Copy link
Contributor

Choose a reason for hiding this comment

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

remove this line. Also <> headers go before ""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

(FWIW: This pattern comes from the Google coding standards; the idea is that each header should be the first header included by some .cpp file, to ensure that each header is properly self-contained.)

vertexai::plaidml::application finalize() const;

// Adds an input to the function.
Function& add(Input input) &;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the problem being solved with returning & and &&? I haven't used this before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was to make it easy to use this as either an lvalue and rvalue-reference, so that you could do either MyObject foo; foo.bar().baz(); return foo.finalize() or return MyObject{}.bar().baz().finalize(), depending on what reads best at the call site.

src/ngraph/runtime/plaidml/plaidml_compilation_cache.hpp Outdated Show resolved Hide resolved
src/ngraph/runtime/plaidml/plaidml_builder.hpp Outdated Show resolved Hide resolved
src/ngraph/runtime/plaidml/plaidml_ops_batch_norm.cpp Outdated Show resolved Hide resolved
@aprocter
Copy link
Contributor

aprocter commented Oct 22, 2018

Should probably designate CODEOWNERS for src/ngraph/runtime/plaidml

@postrational
Copy link
Member

Other external dependencies are automatically downloaded and built though cmake configurations.
Do we want to do the same with PlaidML?

@earhart
Copy link
Contributor Author

earhart commented Oct 23, 2018

We haven't released an SDK for PlaidML yet; it's currently just a Bazel rule that builds a tar file with the right contents. I think we'd like to have a non-Python binary release of some sort, but it's not our highest priority, and I think we'd need to think a bit more about what it'd look like. (E.g. on MacOS, it seems like we might want it to be a Framework.)

It might be possible to download PlaidML and compile it via shelling out to Bazel. I suspect it'd take me a while to get that working, but it might be simple for someone with more expertise with cmake. It'll probably be easier after our next open-source release (hopefully soon); that should include a stable source tarball.

(I very briefly thought about trying to talk nGraph devs into adding a Bazel build system, which would make it trivial to compile PlaidML as part of the build. That seems like a much longer conversation, though, and doesn't really help unless it becomes the only build system.)

Used m_ prefix for members; removed trailing underscores
Updated license headers
Moved associated header inclusions to project blocks
Wrapped comments to 100 chars
Added missing newlines between functions
Removed nested namespaces in operation implementations
@earhart
Copy link
Contributor Author

earhart commented Oct 28, 2018

FWIW -- I don't have access to the Jenkins server; anyone want to let me know what failed? :-/

@aprocter
Copy link
Contributor

@earhart I put in the correct (I think :/) access requests for you. Can take a look at the CI for you later this afternoon if the approval hasn't come through by then.

@earhart
Copy link
Contributor Author

earhart commented Oct 29, 2018

Awesome; thanks! I'll keep checking it periodically.

Copy link
Contributor

@Krovatkin Krovatkin left a comment

Choose a reason for hiding this comment

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

:shipit:

@rkimballn1 rkimballn1 added the Fully Reviewed PR is approved by all reviewers. label Oct 29, 2018
@aprocter
Copy link
Contributor

Looks like @rkimballn1 already got it. The problem was the source code formatting check. You can run maint/check-code-format.sh to check the formatting, and maint/apply-code-format.sh to automatically reformat.

@rkimballn1 rkimballn1 merged commit f0acb7d into master Oct 29, 2018
@rkimballn1 rkimballn1 deleted the rob-plaidml branch October 29, 2018 18:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fully Reviewed PR is approved by all reviewers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants