Add Java 11 support - Initial Phase - #185
Conversation
Sorry, something went wrong.
Sync with master tensorflow on upstream
Merge main branch to local branch
Update after losses merge
Fix Javadoc errors (tensorflow#152)
pull type def
|
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. |
Sorry, something went wrong.
|
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
|
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
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.
|
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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/. |
Sorry, something went wrong.
| <id>jdk11</id> | ||
| <properties> | ||
| <maven.compiler.source>11</maven.compiler.source> | ||
| <maven.compiler.target>11</maven.compiler.target> |
There was a problem hiding this comment.
@JimClarke5 , can you add maven.compiler.release=11 as well, as suggested by @Craigacp ? Then we'll be good to merge it
Sorry, something went wrong.
There was a problem hiding this comment.
Just pushed it.
Sorry, something went wrong.
* Add profile for JDK11 and Automatic-Module-Name to jars * add maven.compiler.release=11
* 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 <rnett@calpoly.edu> * fix style Signed-off-by: Ryan Nett <rnett@calpoly.edu> * move checks to builder method Signed-off-by: Ryan Nett <rnett@calpoly.edu> * Initialization imprvements (#178) * No-op on initAdd in eager mode Signed-off-by: Ryan Nett <rnett@calpoly.edu> * runInit() method in session Signed-off-by: Ryan Nett <rnett@calpoly.edu> * add doInitialization() to Runner Signed-off-by: Ryan Nett <rnett@calpoly.edu> * fix javadoc Signed-off-by: Ryan Nett <rnett@calpoly.edu> * assume only graph or eager environments Signed-off-by: Ryan Nett <rnett@calpoly.edu> * Remove doInit(), update javadocs Signed-off-by: Ryan Nett <rnett@calpoly.edu> * small fixes Signed-off-by: Ryan Nett <rnett@calpoly.edu> * Clairify tensorOf lifetime requirements (#190) * Clairify tensorOf lifetime requirements Signed-off-by: Ryan Nett <rnett@calpoly.edu> * Do codegen Signed-off-by: Ryan Nett <rnett@calpoly.edu> * Remove extra generics from op generation (#193) * Successfully remove extra type params, but it broke javadoc generation Signed-off-by: Ryan Nett <rnett@calpoly.edu> * Generate covariant types Signed-off-by: Ryan Nett <rnett@calpoly.edu> * Do generation Signed-off-by: Ryan Nett <rnett@calpoly.edu> * Update help text. Signed-off-by: Ryan Nett <rnett@calpoly.edu> * Fixes Signed-off-by: Ryan Nett <rnett@calpoly.edu> * 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 <rnett@calpoly.edu> * 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 <rnett@calpoly.edu>
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)
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.