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

Add windows JNI support #44257

Closed
wants to merge 7 commits into from
Closed

Add windows JNI support #44257

wants to merge 7 commits into from

Conversation

erip
Copy link
Contributor

@erip erip commented Sep 6, 2020

Fixes #32516

@dr-ci
Copy link

dr-ci bot commented Sep 6, 2020

💊 CI failures summary and remediations

As of commit e62256e (more details on the Dr. CI page):


  • 4/4 failures possibly* introduced in this PR
    • 2/4 non-CircleCI failure(s)---

2 failures not recognized by patterns:

Job Step Action
CircleCI docker-pytorch-linux-xenial-py3-clang5-android-ndk-r19c Check if image should be built 🔁 rerun
CircleCI pytorch_linux_xenial_py3_6_gcc5_4_build Build 🔁 rerun

🚧 1 ongoing upstream failure:

These were probably caused by upstream breakages that are not fixed yet:


ci.pytorch.org: 2 failed


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 157 times.

@erip
Copy link
Contributor Author

erip commented Sep 6, 2020

I'm not positive if there's something special I need to do to ensure a JDK is installed on the CI runners, but the usual suspects on Windows (C:\Program Files\Java, C:\Program Files\AdoptOpenJDK) aren't resolving to JAVA_HOMEs. 😬

Copy link
Collaborator

@mszhanyi mszhanyi left a comment

Choose a reason for hiding this comment

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

sorry, ignore it

Copy link
Collaborator

@mszhanyi mszhanyi left a comment

Choose a reason for hiding this comment

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

Fixes #44363

@mszhanyi mszhanyi requested review from mszhanyi and removed request for mszhanyi September 17, 2020 03:23
@erip
Copy link
Contributor Author

erip commented Sep 17, 2020

This is not quite ready, so I'd remove approval @mszhanyi. The fbjni folks have also been responding, so this may be moving a bit more soon. 🙌

@mszhanyi
Copy link
Collaborator

I'm not positive if there's something special I need to do to ensure a JDK is installed on the CI runners, but the usual suspects on Windows (C:\Program Files\Java, C:\Program Files\AdoptOpenJDK) aren't resolving to JAVA_HOMEs. 😬
No problem. I just don't find how to remove me from the reviewer list.

@mszhanyi mszhanyi requested review from mszhanyi and removed request for mszhanyi September 17, 2020 10:45
@erip
Copy link
Contributor Author

erip commented Sep 25, 2020

Lingering TODOs:

  • conditionally link optional libraries on Windows.
  • Find out why libtorch build is failing on Linux. 🤔

@erip
Copy link
Contributor Author

erip commented Sep 25, 2020

cc @dreiss do these fbjni errors look familiar to you? They they all look related and this is the history of SimpleFixedString.h - not sure why it only fails in these limited cases... It seems like SimpleFixedString is claiming that the strings aren't null-terminated which is causing an (intentional) compilation error.

Edit: I think the fix is to update Meta.h L306 like what's demonstrated in this godbolt: https://godbolt.org/z/sYffn6

WDYT @dreiss?

@erip erip changed the title [WIP] Add windows JNI support Add windows JNI support Sep 25, 2020
@zou3519 zou3519 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Sep 25, 2020
@erip
Copy link
Contributor Author

erip commented Sep 25, 2020

@mszhanyi I don't imagine this PR is going to change dramatically by fixing the remaining two build errors, so if you'd like to review it I think this PR is quite close.

@erip erip requested a review from mszhanyi September 25, 2020 17:14
@erip
Copy link
Contributor Author

erip commented Sep 25, 2020

Waiting for this PR to be merged.

@erip
Copy link
Contributor Author

erip commented Sep 26, 2020

I think these errors are a fixture of flaky builds and upstream breakages. I'll try to rebuild a bit later.

@erip
Copy link
Contributor Author

erip commented Sep 26, 2020

5600e82 is a temporary commit while we wait for the commits to land in fbjni.

Copy link
Collaborator

@peterjc123 peterjc123 left a comment

Choose a reason for hiding this comment

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

These changes LGTM.

@codecov
Copy link

codecov bot commented Sep 27, 2020

Codecov Report

Merging #44257 into master will decrease coverage by 0.04%.
The diff coverage is 60.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #44257      +/-   ##
==========================================
- Coverage   68.31%   68.26%   -0.05%     
==========================================
  Files         410      410              
  Lines       53582    53614      +32     
==========================================
- Hits        36602    36598       -4     
- Misses      16980    17016      +36     
Impacted Files Coverage Δ
torch/nn/quantized/modules/__init__.py 97.22% <ø> (ø)
torch/testing/_internal/expecttest.py 78.57% <ø> (+1.02%) ⬆️
...orch/testing/_internal/distributed/rpc/rpc_test.py 26.39% <14.28%> (-0.07%) ⬇️
.../testing/_internal/distributed/rpc/jit/rpc_test.py 28.97% <18.18%> (-0.17%) ⬇️
torch/futures/__init__.py 84.61% <50.00%> (-2.89%) ⬇️
torch/jit/annotations.py 91.78% <50.00%> (-0.39%) ⬇️
torch/serialization.py 86.87% <75.00%> (+0.56%) ⬆️
torch/quantization/fx/quantize.py 96.24% <90.90%> (-0.17%) ⬇️
torch/optim/_multi_tensor/adam.py 94.87% <100.00%> (ø)
torch/optim/_multi_tensor/adamw.py 93.50% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d6fd22...e62256e. Read the comment docs.

@erip
Copy link
Contributor Author

erip commented Oct 13, 2020

@peterjc123 @mszhanyi I think this is good once CI passes. WDYT?

Copy link
Collaborator

@peterjc123 peterjc123 left a comment

Choose a reason for hiding this comment

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

Again, this LGTM.

@peterjc123
Copy link
Collaborator

Please merge this pr. @ezyang @malfet

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@erip
Copy link
Contributor Author

erip commented Oct 15, 2020

These errors seem to be related to git funniness because of a rebase - seems to be a fluke and should be OK to merge.

@erip erip deleted the feature/add-windows-jni-support branch October 15, 2020 18:12
@facebook-github-bot
Copy link
Contributor

@ezyang merged this pull request in b547973.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merged open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Java bindings for Windows
7 participants