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

nGraph Preset initial commit #642

Merged
merged 53 commits into from
Jan 18, 2019
Merged

nGraph Preset initial commit #642

merged 53 commits into from
Jan 18, 2019

Conversation

EmergentOrder
Copy link
Member

Another preset incoming!

So I've managed to get it down to just one error on the jningraph.cpp file:

jningraph.cpp:9879:60: error: ‘position1’ was not declared in this scope
         env->SetLongField(arg1, JavaCPP_limitFID, rsize1 + position1);

Any idea what's going on here? This is in the class NgraphResultVector. There are several other NgraphSOMEVector, defined similarly, which don't seem to have the same issue.

@saudet
Copy link
Member

saudet commented Nov 19, 2018

Thanks! The issue might have something to do with recent changes in commit bytedeco/javacpp@f214f5c? But there isn't any buffers used in the class... I guess I'll have to look at this.

@EmergentOrder
Copy link
Member Author

After adding runtime::Tensor and runtime::Backend, now I get 2 position related errors.
Also, we are missing the from<SOME> functions from ngraph::element::Type, because they failed to parse (see element_type.hpp.patch).

@saudet
Copy link
Member

saudet commented Nov 20, 2018

BTW, add an entry to .travis.yml to have the build tested at least on Linux.

@EmergentOrder
Copy link
Member Author

Added BackendManager, which raises these new errors:

error: ‘void*’ is not a pointer-to-object type
         ::ngraph::onnxifi::BackendManager::unregister(*ptr0);`
 error: ‘void*’ is not a pointer-to-object type
         rptr = (const ::ngraph::onnxifi::Backend*)&::ngraph::onnxifi::BackendManager::get(*ptr0);

@saudet
Copy link
Member

saudet commented Nov 27, 2018

I'm not able to build nGraph with cppbuild.sh, it fails with this error (Fedora 27):

[ 60%] Linking CXX shared library libcodegen.so
/usr/bin/ld: cannot find -ltinfo
collect2: error: ld returned 1 exit status
make[2]: *** [src/ngraph/codegen/CMakeFiles/codegen.dir/build.make:188: src/ngraph/codegen/libcodegen.so] Error 1
make[1]: *** [CMakeFiles/Makefile2:759: src/ngraph/codegen/CMakeFiles/codegen.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[ 61%] Linking CXX shared library ../../libinterpreter_backend.so
[ 61%] Built target interpreter_backend
make: *** [Makefile:152: all] Error 2

Could you first get the cppbuild.sh script working? Also, you're missing the "header" to ensure that it builds under the cppbuild subdirectory, and let's use the download function instead of git as per what we did for ONNX:
https://github.com/bytedeco/javacpp-presets/blob/master/onnx/cppbuild.sh
Thanks for your time on this!

@EmergentOrder
Copy link
Member Author

Followed the instructions for CentOS, on Fedora 27 (in Docker) and I get a successful build (up to the errors I reported above).

Without installing the required packages in Fedora, I got errors before even reaching cannot find -ltinfo, but I guess you have those requirements already. It looks like tinfo is in ncurses.

So I'll make install ncurses in cppbuild.sh, not sure if that's the only thing that I need to add though.

@EmergentOrder
Copy link
Member Author

Works for me now on Fedora 27 without package ncurses-devel.
Working on your end?

@saudet
Copy link
Member

saudet commented Nov 28, 2018

Hum, it needs ncurses, ok, that's probably not going to work on Mac anytime soon, but that's alright. Let's see what we can do on Linux first.

@saudet
Copy link
Member

saudet commented Nov 28, 2018

A few more things to fix with the build:

  • We're building ngraph not for onnx, so let's update that line in cppbuild.sh
  • It installs in the $HOME/ngraph_dist directory, let's change that to the cppbuild/<platform> directory

saudet added a commit to bytedeco/javacpp that referenced this pull request Nov 28, 2018
@saudet
Copy link
Member

saudet commented Nov 28, 2018

In other news, I've fixed compilation failing with error: ‘position1’ was not declared in this scope in commit bytedeco/javacpp@99835a5. For the other one with onnxBackendID, the problem is that it's an @Opaque type and it's trying to call functions that need its definition with @ByVal. You'll need to either skip those functions, or include a header file with the definition for onnxBackendID.

@EmergentOrder
Copy link
Member Author

EmergentOrder commented Nov 28, 2018

Awesome, thanks for the fix.
I'll take a look at onnxBackendID.
What about the parsing fail on the from<X> functions in element_type.hpp (currently patched out, but I will remove the patching so it surfaces, because we need those). Can you take a look? Thanks.

@EmergentOrder
Copy link
Member Author

Actually, it looks like it parses element_type.hpp ok, but then fails on the generated java.

@saudet
Copy link
Member

saudet commented Nov 29, 2018

We just need to give them names like:

.put(new Info("ngraph::element::from<char>").javaNames("fromChar"));
...

See https://github.com/bytedeco/javacpp/wiki/Mapping-Recipes#specifying-names-to-use-in-java and https://github.com/bytedeco/javacpp/wiki/Mapping-Recipes#creating-instances-of-c-templates

BTW, since it appears to install everything properly in the install path, we probably don't need to copy everything manually.

saudet added a commit to bytedeco/javacpp that referenced this pull request Jan 4, 2019
@saudet
Copy link
Member

saudet commented Jan 4, 2019

Sure thing, fixed in commit bytedeco/javacpp@2cf75e5!

@EmergentOrder
Copy link
Member Author

The last build failed, not on parsing this time, but on the generated Java.

@saudet
Copy link
Member

saudet commented Jan 6, 2019

Yes, we need more Info.

@saudet
Copy link
Member

saudet commented Jan 8, 2019

The current error shouldn't be hard to resolve, but as usual, let me know if you need help to make progress.

@EmergentOrder
Copy link
Member Author

EmergentOrder commented Jan 13, 2019

No problem, I'll take care of that.

Another issue, brought by further changes I haven't checked in yet:
error: cannot convert ‘const Operator* {aka const std::function<ngraph::NodeVector(const ngraph::onnx_import::Node&)>*}’ to ‘ngraph::NodeVector (*)(const ngraph::onnx_import::Node&)’ in assignment

Where I am declaring my own FunctionPointer for Operator.
I tried adding @Const to the declaration, but I get a bunch of other errors related to const/volatile.

This on the generated C++ for this function: const Operator& get_operator(const std::string& name, const std::string& domain) const

Any ideas?

@saudet
Copy link
Member

saudet commented Jan 14, 2019 via email

@EmergentOrder
Copy link
Member Author

hmm, ok, looking for an alternative to get_operator,
I found @Namespace("ngraph::onnx_import") public static native @ByVal NgraphFunctionVector load_onnx_model(@Cast("std::istream*") @ByRef Pointer sin);

How can I load a file into a Pointer containing an istream so I can call this?

@saudet
Copy link
Member

saudet commented Jan 15, 2019 via email

@EmergentOrder
Copy link
Member Author

EmergentOrder commented Jan 15, 2019

This is what I had for Operator:

       public static class Operator extends FunctionPointer {
        static { Loader.load(); }
        /** Pointer cast constructor. Invokes {@link Pointer#Pointer(Pointer)}. */
        public    Operator(Pointer p) { super(p); }
        protected Operator() { allocate(); }
        private native void allocate();
        public @ByVal native @Cast("ngraph::NodeVector*") Pointer call(@ByRef @Cast("const ngraph::onnx_import::Node*") Pointer a);
   }

so I did define it manually, but got the error when trying to use it.
I'm not clear on how generated custom call() methods would help the cannot convert error I hit. It looked to me like the issue was just not being able to assign non-const <- const in the generated C++.

@saudet
Copy link
Member

saudet commented Jan 15, 2019

No, I mean defining mappings for the std::function instance itself, like

@Name("std::function<something>") class SomeFunction {
    @Name("operator()") public native void call(...);
    ...
}

And to access generated types with it, either as part of a helper class or with some Info.javaText:
https://github.com/bytedeco/javacpp/wiki/Mapping-Recipes#mapping-a-declaration-to-custom-code
https://github.com/bytedeco/javacpp/wiki/Mapping-Recipes#writing-additional-code-in-a-helper-class

@EmergentOrder
Copy link
Member Author

This is ready to merge.
I will likely follow up with separate PR(s) with more headers as needed, particularly when nGraph ONNXIFI support comes.

ngraph/CMakeLists.txt.patch Outdated Show resolved Hide resolved
ngraph/cppbuild.sh Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@saudet saudet merged commit fb79284 into bytedeco:master Jan 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants