Skip to content

Add Java 11 support - Initial Phase #185

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

Merged
merged 7 commits into from
Jan 26, 2021
Merged

Conversation

JimClarke5
Copy link
Contributor

This PR adds support for compiling with a JDK 11 compiler and producing JDK 11 class files. This does not fully modularize the system per the specs in the Java Platform Module System (JPMS), but merely allows the existing jar files to play nicely in a JDK 11 environment as Automatic Modules.

I added a profile to compile to Java 11 classes. To use it, you need a JDK 11 installation and use the maven profile jdk11,
e.g. mvn -Pjdk11 install. This profile will change the compile source and target from 1.8 to 11.

In addition, I added an Automatic-Module-Name entry into each jar's manifest. The was done in each POM that generates a jar and allows JDK 11 environments to clearly define Automatic Modules. In each POM that generates jars, I created a property, java.module.name, to hold the name. Then I modified the maven-jar-plugin in each POM to include the Manfiest Entry similar to the following:

      <manifestEntries>
              <Automatic-Module-Name>${java.module.name}</Automatic-Module-Name>
     </manifestEntries>

Here are the names I selected for the modules. These are easy to change if needed, and this follows the suggested reverse Internet domain-name convention for modules. See Proposal: #AutomaticModuleNames (revised)

  • <java.module.name>org.tensorflow.core.platform.gpu</java.module.name>
    
  • <java.module.name>org.tensorflow.core.platform.mkl</java.module.name>
    
  • <java.module.name>org.tensorflow.core.api</java.module.name>
    
  • <java.module.name>org.tensorflow.core.generator</java.module.name>
    
  • <java.module.name>org.tensorflow.core.platform.mkl</java.module.name>
    
  • <java.module.name>org.tensorflow.core.platform</java.module.name>
    
  • <java.module.name>org.tensorflow.framework</java.module.name>
    
  • <java.module.name>org.tensorflow.ndarray</java.module.name>
    

NOTE: This does not fully modularize the system, but merely allows the jar files to play nicely in an JDK 11 environment.
To modularize the Jars, we would need to create a module-info.java for each Jar. The module-info.java file is only recognized by JDK versions 9 and higher, so this creates some issues with our current development environment.
There is a way to to create a Jar that supports multi-versions, so we could create a Jar that supports both 8 and 11. However, this requires JDK 9 or higher to build the system. In a nutshell, to fully go JPMS, we would have to require JDK 11 for all compilation, though we could issue Java 8 and Java 11 combined jars. We need to discuss this before making that requirement. Also, modularizing the jars will probably not be a trivial task, as JPMS enforces stricter encapsulation rules.

@Craigacp
Copy link
Collaborator

Is it worth moving the CI over to use Java 11 and then set the release flag to 8 or 11 in the profile? That'll be better at preventing us from using Java 11 language features before we're ready.

@saudet
Copy link
Contributor

saudet commented Jan 10, 2021 via email

@JimClarke5
Copy link
Contributor Author

Is it worth moving the CI over to use Java 11 and then set the release flag to 8 or 11 in the profile? That'll be better at preventing us from using Java 11 language features before we're ready.

I thought about this after I created this PR. The goal of this exercise was to see if we could play nicely with JDK 11 developers. I think if we set the source and target to 8, while keeping the Automatic Module names, accomplishes that. Without fully embracing JPMS, I think this is enough for now. Should we take out the jdk11 profile until we are ready to make the move to Java 11 specific features? You can still compile using JDK 11, or even later, if you want, as these later releases already handle Java 8 sources and targets.

@JimClarke5
Copy link
Contributor Author

BTW, if the reason to use Java 11 is solely for modules, we can do that with Java SE 8 and ModiTect: https://github.com/moditect/moditect Check, for example, JavaCPP: https://github.com/bytedeco/javacpp

The goal here was to see if we could play nicely with JDK 9+ developers. It wasn't to fully modularize, though that should be done at a later time. There are still a significant number of Java 8 projects to support, and that will probably be the case for some time. I would think we would want to keep the project as simple as could be for now and support standard Java features as much as we can.

@saudet
Copy link
Contributor

saudet commented Jan 11, 2021 via email

@Craigacp
Copy link
Collaborator

Is it worth moving the CI over to use Java 11 and then set the release flag to 8 or 11 in the profile? That'll be better at preventing us from using Java 11 language features before we're ready.

I thought about this after I created this PR. The goal of this exercise was to see if we could play nicely with JDK 11 developers. I think if we set the source and target to 8, while keeping the Automatic Module names, accomplishes that. Without fully embracing JPMS, I think this is enough for now. Should we take out the jdk11 profile until we are ready to make the move to Java 11 specific features? You can still compile using JDK 11, or even later, if you want, as these later releases already handle Java 8 sources and targets.

I think the release flag is better than setting source and target as it prevents us from using method calls not present in versions after release, though it's only available on 9+ (but you can use -release 8 on 9+). I'm fine with the existence of the jdk11 profile, but I'm not sure of the value of it given everything should be upwards compatible. I use the latest JDK as my dev environment and I've not had any TF-java issues. Admittedly I've not tried to modularise anything beyond adding automatic module names yet.

@Craigacp
Copy link
Collaborator

Ah, if it's for testing with Java 11, we could add a job to the CI for that. Or the other way around may work as well, deploy with Java 11 and test using Java 8 with the CI.

I agree with this. I test Tribuo on 8, 11 and latest using the CI, and we could at least run the quick CI on all of them, even if we don't do the full ops regeneration & build.

@karllessard
Copy link
Collaborator

In the CI, we only need to build that last step (deploy) or the quick build (triggered by PRs) with JDK 8 and/or 11. I like the idea of giving a try to multi-version jars but we can do that at a later step. It could also be possible in the quick build for PRs to build both versions, to validate that we don't break anything and that no JDK11 features are being used yet.

Am I right to assume that it does not matter which JDK we use for the native builds as we only distributes native binaries in the org.tensorflow:tensorflow-core-api:*? Is there any incompatible differences in the JNI layer or the auto-generated JavaCPP native code that would require us to provide different builds as well?

@saudet
Copy link
Contributor

saudet commented Jan 12, 2021

No, there's nothing in JavaCPP or JNI that breaks with Java 11. Right now, OpenJDK 8 gets used to compile all the classes except module-info.class, which ModiTect creates and places under META-INF so they get ignored by versions of the JDK less than 9. The resulting JAR files work fine with both Java 8 and 11, including with the module path.

@Craigacp
Copy link
Collaborator

Craigacp commented Jan 16, 2021

I came across this blog post via JetBrains' monthly Java roundup and it's got a nice succinct explanation of why we should try to use the release flag when building on newer JDKs but targeting 8. I'd seen this ByteBuffer issue mentioned in various places on Github but this writeup captures everything - https://www.morling.dev/blog/bytebuffer-and-the-dreaded-nosuchmethoderror/.

<id>jdk11</id>
<properties>
<maven.compiler.source>11</maven.compiler.source>
<maven.compiler.target>11</maven.compiler.target>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@JimClarke5 , can you add maven.compiler.release=11 as well, as suggested by @Craigacp ? Then we'll be good to merge it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed it.

@karllessard karllessard merged commit d6d07cd into tensorflow:master Jan 26, 2021
JimClarke5 added a commit to JimClarke5/java that referenced this pull request Jan 30, 2021
* Add profile for JDK11 and  Automatic-Module-Name to jars

* add maven.compiler.release=11
karllessard pushed a commit that referenced this pull request Feb 1, 2021
* Initial checkin

* Initial checkin and sync with master

* Initial checkin and sync with master

* JavaDoc cleanup

* Javadoc fixes

* Change LossInterface to LossMetric.
Fix JavaDoc,
modify one line code block to include braces.

* Removed hashmap for variables, they are not needed as the variables only live within a single instance of a Metric.

* reformat code

* Add tests for assertBroadcastable

* Change type to resultType

* Added V data type for sampleWeights so that it is not forced to be the same type as the return or internal variables,

* change 'type' to 'resultType'

* clean up mean and fix assert assertBroadcastable

* fix error message

* Change sampleWeights to have its own generic type <S extends TNumber>

* Add commment about invalid tests expecting IllegalArgumentExceptions

* Add this exception instead of the more generic IllegalArgumentException when static shapes cannot boradcast.

* change IllegalArgumentException to NotBroadcastableException.
change hasValidNonscalarShape to canBroadcastNonscalarShapes
change hasValidNonscalarShape to canBroadcastNonscalarShapes

* reformat code

* Fis=x Javadoc
move the dynamic shapes and rank down to the dynamic section so they are created needlessly when static
Fix if statement to check for unknown size and unknown dimensions

* Fix Reduce to use boradcastWeights,
renamed WeightBroadcastTest to AssertBroadcastableTest and added BroadcastWeightsTest

* Added comment to count to indicate that it may be weighted.

* Added SetsOps and fixed AssertBroadcastable to use SetsOps methods,

* Fixed based on various PR comments.

* Deleted, no longer needed after change to Variable handling in Metrics.

* Nicer error messages for mode-forbidden ops (#169)

* start fobbiden ops checks

Signed-off-by: Ryan Nett <[email protected]>

* fix style

Signed-off-by: Ryan Nett <[email protected]>

* move checks to builder method

Signed-off-by: Ryan Nett <[email protected]>

* Initialization imprvements (#178)

* No-op on initAdd in eager mode

Signed-off-by: Ryan Nett <[email protected]>

* runInit() method in session

Signed-off-by: Ryan Nett <[email protected]>

* add doInitialization() to Runner

Signed-off-by: Ryan Nett <[email protected]>

* fix javadoc

Signed-off-by: Ryan Nett <[email protected]>

* assume only graph or eager environments

Signed-off-by: Ryan Nett <[email protected]>

* Remove doInit(), update javadocs

Signed-off-by: Ryan Nett <[email protected]>

* small fixes

Signed-off-by: Ryan Nett <[email protected]>

* Clairify tensorOf lifetime requirements (#190)

* Clairify tensorOf lifetime requirements

Signed-off-by: Ryan Nett <[email protected]>

* Do codegen

Signed-off-by: Ryan Nett <[email protected]>

* Remove extra generics from op generation (#193)

* Successfully remove extra type params, but it broke javadoc generation

Signed-off-by: Ryan Nett <[email protected]>

* Generate covariant types

Signed-off-by: Ryan Nett <[email protected]>

* Do generation

Signed-off-by: Ryan Nett <[email protected]>

* Update help text.

Signed-off-by: Ryan Nett <[email protected]>

* Fixes

Signed-off-by: Ryan Nett <[email protected]>

* Add Java 11 support - Initial Phase (#185)

* Add profile for JDK11 and  Automatic-Module-Name to jars

* add maven.compiler.release=11

* Update manual ops for new codegen (#196)

Signed-off-by: Ryan Nett <[email protected]>

* Fix Losses to use CHANNELS_FIRST/LAST for CategoricalCrossentropy

* Fix SetOps to properly convert sparse tensor to dense tensor using tf.sparse.sparseToDense with the output of tf.sparse.denseToDenseSetOperation

* Initial checkin

* Initial checkin and sync with master

* Initial checkin and sync with master

* JavaDoc cleanup

* Javadoc fixes

* Change LossInterface to LossMetric.
Fix JavaDoc,
modify one line code block to include braces.

* Removed hashmap for variables, they are not needed as the variables only live within a single instance of a Metric.

* reformat code

* Add tests for assertBroadcastable

* Change type to resultType

* Added V data type for sampleWeights so that it is not forced to be the same type as the return or internal variables,

* change 'type' to 'resultType'

* clean up mean and fix assert assertBroadcastable

* fix error message

* Change sampleWeights to have its own generic type <S extends TNumber>

* Add commment about invalid tests expecting IllegalArgumentExceptions

* Add this exception instead of the more generic IllegalArgumentException when static shapes cannot boradcast.

* change IllegalArgumentException to NotBroadcastableException.
change hasValidNonscalarShape to canBroadcastNonscalarShapes
change hasValidNonscalarShape to canBroadcastNonscalarShapes

* reformat code

* Fis=x Javadoc
move the dynamic shapes and rank down to the dynamic section so they are created needlessly when static
Fix if statement to check for unknown size and unknown dimensions

* Fix Reduce to use boradcastWeights,
renamed WeightBroadcastTest to AssertBroadcastableTest and added BroadcastWeightsTest

* Added comment to count to indicate that it may be weighted.

* Added SetsOps and fixed AssertBroadcastable to use SetsOps methods,

* Fixed based on various PR comments.

* Deleted, no longer needed after change to Variable handling in Metrics.

* Fix Losses to use CHANNELS_FIRST/LAST for CategoricalCrossentropy

* Fix SetOps to properly convert sparse tensor to dense tensor using tf.sparse.sparseToDense with the output of tf.sparse.denseToDenseSetOperation

Co-authored-by: Ryan Nett <[email protected]>
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.

4 participants