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

ONNX (v1.2.2) preset #547

Merged
merged 60 commits into from
Aug 4, 2018
Merged

ONNX (v1.2.2) preset #547

merged 60 commits into from
Aug 4, 2018

Conversation

EmergentOrder
Copy link
Member

No description provided.

@saudet
Copy link
Member

saudet commented Apr 7, 2018

Looking good, thanks! Could you also do the following though?

  1. Add an entry for at least linux-x86_64 to .travis.yml
  2. Move the lines about installing miniconda to install-travis.sh in the ci subdirectory
  3. Make a patch file out of schema.h instead of including the whole file

And let's see what Travis CI has to say about that before we can merge! Thanks

@EmergentOrder
Copy link
Member Author

Done!
Thanks for the help along the way

@EmergentOrder
Copy link
Member Author

@saudet Just when I think I finally have these build issues fixed, something new pops up.
Specifically,

[ERROR] /javacpp-presets/onnx/src/main/java/org/bytedeco/javacpp/onnx.java:[1187,16] repeated annotations are not supported in -source {0}
[ERROR] (use -source 8 or higher to enable repeated annotations)
[ERROR] /javacpp-presets/onnx/src/main/java/org/bytedeco/javacpp/onnx.java:[1189,16] org.bytedeco.javacpp.annotation.StdString is not a repeatable annotation type

This is happening today for the first time, only with JavaCPP 1.4.2-SNAPSHOT. When I point to 1.4.1, no compile error (locally).
I can log an issue, but I thought I'd just give you the heads up first (as I think this will fail my next CI build here)

@saudet
Copy link
Member

saudet commented Apr 13, 2018

Oops, sorry about, fixed: bytedeco/javacpp@be49bde

BTW, installing miniconda on the host with apt-get probably won't work. The builds happen in Docker containers.

Also, FYI, for faster turnaround with CI we can temporarily disable all other builds in .travis.yml and .appveyor.yml.

@EmergentOrder
Copy link
Member Author

Yeah, the build was taking forever, thanks for the suggestion, I disabled the other projects.
Also, I actually don't think I need apt-get or anything at all in install-travis.sh now.
There are a lot of commits here now, from tangling with the CI. If you want, I can put this all in a single commit (once it passes) to clean up the history.

@saudet
Copy link
Member

saudet commented Apr 14, 2018 via email

@EmergentOrder
Copy link
Member Author

@saudet ok, I got past the default_instance declaration errors by skipping. I still get these 2 errors though:

javacpp-presets/onnx/target/classes/org/bytedeco/javacpp/jnionnx.cpp:1452:51: error: expression cannot be used as a function
         StringAdapter radapter(ptr->string_value_());

and

javacpp-presets/onnx/target/classes/org/bytedeco/javacpp/jnionnx.cpp:1487:46: error: expression cannot be used as a function
         rptr = ptr->string_value_((char*)ptr0);```

@EmergentOrder
Copy link
Member Author

once again, it looks like skip did the trick. I'll push my changes now

@EmergentOrder
Copy link
Member Author

@saudet should be good to go

@saudet
Copy link
Member

saudet commented Jun 19, 2018

Thanks! Looks like ONNX 1.2.2 is out. Could we update the presets?

@saudet
Copy link
Member

saudet commented Jun 19, 2018

Would you also have some sample code we could put in a README.md file?

@EmergentOrder
Copy link
Member Author

I had a branch with 1.1.0 ready to go, looks like I can bump that to 1.1.2 without issue. I'll push the merged changes from that now.
1.2.2 has errors, so I'll look at those at some point when I have a moment.

@EmergentOrder EmergentOrder changed the title ONNX (v1.0.1) preset ONNX (v1.1.2) preset Jul 3, 2018
@EmergentOrder
Copy link
Member Author

1.2.2 builds now but throws a runtime error in my first test with the jar

@EmergentOrder
Copy link
Member Author

looks like an issue with an rvalue reference that was introduced

@saudet
Copy link
Member

saudet commented Jul 6, 2018 via email

@saudet
Copy link
Member

saudet commented Jul 9, 2018

BTW, could you add a README.md file with your sample code in it?

@saudet
Copy link
Member

saudet commented Aug 1, 2018

I've been fixing and cleaning this up, and now it looks good to merge! I've also changed the license to match the other presets, so please let me know if this not OK with you. Thanks!

@EmergentOrder
Copy link
Member Author

Thanks for that, it looks good at a first glance.

I'm still getting the runtime error I mentioned before, on this call:
org.bytedeco.javacpp.onnx.OpSchemaRegistry.get_all_schemas()
This call is essential for my application. Works in 1.1.2 and older, pre-rvalue.

The other issue is new with your changes, will comment inline

@@ -1194,7 +1192,7 @@ private native void allocate(
// Get output formal parameters.
public native @Const @ByRef FormalParameterVector outputs();

public native @Const @ByRef TypeConstraintParamVector typeConstraintParams();
public native @StdVector TypeConstraintParam typeConstraintParams();
Copy link
Member Author

@EmergentOrder EmergentOrder Aug 2, 2018

Choose a reason for hiding this comment

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

Why does this no longer return a vector (TypeConstraintParamVector) ? This type change breaks my code (and no longer reflects schema.h).

saudet added a commit to bytedeco/javacpp that referenced this pull request Aug 3, 2018
…onst ` prefix (issue bytedeco/javacpp-presets#595)

 * Fix `Parser` sometimes ignoring `define` of `const ` containers (pull bytedeco/javacpp-presets#547)
@saudet
Copy link
Member

saudet commented Aug 3, 2018

Ok, I've fixed that and it seems to work fine now. Thanks for testing!

@EmergentOrder
Copy link
Member Author

Great, both issues are fixed now, thanks!
Now I only get errors due to changes in ONNX internals in the more recent versions, but that's something else entirely...
I approve the license change.
Good to go.

@EmergentOrder EmergentOrder changed the title ONNX (v1.1.2) preset ONNX (v1.2.2) preset Aug 3, 2018
@saudet saudet merged commit cd7ac05 into bytedeco:master Aug 4, 2018
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