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

patch for --EmitJNI on windows #2582

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

patch for --EmitJNI on windows #2582

wants to merge 2 commits into from

Conversation

armgong
Copy link

@armgong armgong commented Oct 25, 2023

this naive fix for the issue 2580 https://github.com/onnx/onnx-mlir/issues/2580
test with add.onnx on windows (jdk 11.0)

part 1 of emitjni on windows

Signed-off-by: Yu Gong <[email protected]>
part 2 of emitjni on windows

Signed-off-by: Yu Gong <[email protected]>
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@tungld
Copy link
Collaborator

tungld commented Oct 25, 2023

@jenkins-droid test this please

@tungld tungld requested a review from gongsu832 October 25, 2023 04:30
@tungld tungld linked an issue Oct 25, 2023 that may be closed by this pull request
Copy link
Collaborator

@AlexandreEichenberger AlexandreEichenberger left a comment

Choose a reason for hiding this comment

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

LGTM but I am not an expert. Requested @gongsu832 to double check as he is the one that developed this code.

Thanks for the quick fix.

@AlexandreEichenberger
Copy link
Collaborator

please run the clang format on the modified files.

@gongsu832
Copy link
Collaborator

For the --EmitJNI option considered to be "working", it should at least pass the JNI backend tests, for which the make targets are:

check-onnx-backend-jni
check-onnx-backend-dynamic-jni
check-onnx-backend-constant-jni
check-onnx-backend-node-jni
check-onnx-backend-model-jni

These tests are currently not run on the Windows CI.

@armgong
Copy link
Author

armgong commented Oct 26, 2023

For the --EmitJNI option considered to be "working", it should at least pass the JNI backend tests, for which the make targets are:

check-onnx-backend-jni
check-onnx-backend-dynamic-jni
check-onnx-backend-constant-jni
check-onnx-backend-node-jni
check-onnx-backend-model-jni

These tests are currently not run on the Windows CI.

yeah, try run these tests manually, but the tests themself are no go.

@AlexandreEichenberger
Copy link
Collaborator

yeah, try run these tests manually, but the tests themself are no go.

@armgong adding these tests is key to make sure your patch continues to work. Would be great if you could look into adding them permanently once you have a clean build with these additional tests. We have no window servers where we can try this out, so we are reliant on you to perform this task. Your effort is much appreciated.

@armgong
Copy link
Author

armgong commented Oct 26, 2023

@AlexandreEichenberger
will try add test for this, but need more time and learn how to add test use python

@hamptonm1
Copy link
Collaborator

@armgong Were you still working on this PR? :)

@armgong
Copy link
Author

armgong commented Oct 2, 2024

sorry, busy for other things, have no time to mod and finished the test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

onnx-mlir --EmitJNI failed on windows, run wrong command
6 participants