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

Create OSGi bundles for ND4J #8083

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

timothyjward
Copy link

What changes were proposed in this pull request?

As described in #3022 it is necessary for Deeplearning4J to be provided as OSGi bundles in order for it to be used embedded in Eclipse. OSGi has a very different class loader structure which impacts how DL4J loads its backend implementation and its various plugins.

This PR makes the necessary additions/fixes to provide ND4J as OSGi bundles (the first step on the road to running the full DL4J on OSGi. Ideally the existing ND4J jars would have been enhanced with OSGi metadata, but this is not possible because:

  • The ND4J API has split packages between its various modules, these cannot work in separate OSGi bundles
  • The ND4J API is tightly coupled to JavaCPP, which further requires a flat class space to load native libraries

The PR therefore creates an nd4j-osgi bundle which wraps up the "frontend api" modules into a single OSGi bundle JAR. This has a requirement for a "backend". There is also an nd4j-native-osgi bundle providing the backend - this is an OSGi fragment which attaches to the frontend API to provide a working backend.

The overall approach is minimally invasive to the existing ND4J code, there are no API changes and all of the existing jars are still produced as before.

How was this patch tested?

There is an automated OSGi sniff test which has been put in the nd4j-tests-osgi module

Quick checklist

The following checklist helps ensure your PR is complete:

  • Eclipse Contributor Agreement signed, and signed commits - see IP Requirements page for details
  • Reviewed the Contributing Guidelines and followed the steps within.
  • Created tests for any significant new code additions.
  • Relevant tests for your changes are passing.

* Minimal changes to the module build, although it appears that this could be simplified
* All Jackson packages exported at the jackson.version

Signed-off-by: Tim Ward <timothyjward@apache.org>
This is the least invasive option for creating an OSGi enabled ND4J bundle, but it could be improved

* Multiple API jars are combined to avoid split package problems, ideally there would be no split packages
* API packages and their versions are determined here, ideally this would be done using annotations in the packages themselves
* Some third party code is included because it is not packaged in OSGi bundles
* Some third party code leaks out through the ND4J API

Signed-off-by: Tim Ward <timothyjward@apache.org>
This is the least invasive option for creating an ND4J backend

* The Native Java API, Java Implementation and Native code are all combined into one module
* Each platform creates its own specific OSGi bundle complete with native dependencies
* The Frontend has a requirement on at least one backend being present
* The Frontend logic has been improved to handle backend discovery/loading when it's a separate module
* A simple OSGi sniff test has been added to demonstrate loading an ND Array

Signed-off-by: Tim Ward <timothyjward@apache.org>
@saudet
Copy link
Contributor

saudet commented Aug 23, 2019

I don't see these changes as "minimally invasive". What prevents us from creating an nd4j-osgi module like the nd4j-uberjar module that doesn't require any changes to the other modules?

@timothyjward
Copy link
Author

I don't see these changes as "minimally invasive". What prevents us from creating an nd4j-osgi module like the nd4j-uberjar module that doesn't require any changes to the other modules?

So in this PR:

  • The ND4J repackaging of Jackson is enhanced with OSGi metadata in the manifest, but there are no other changes
  • There are some internal ND4J changes to allow ServiceLoader to find implementations of things when running in OSGi, vanilla Java usage is unaffected
  • There is a new ND4J "frontend" Maven Module which packages up the ND4J core, common, context and buffer
  • There is a new ND4J "native backend" Maven Module which packages up the native-api and native libraries for the CPU backend
  • There is a new ND4J "OSGi test" project which runs tests in the OSGi environment

Of these the only ones which affect existing modules are the internal ServiceLoader changes and Jackson packaging changes, both of which are necessary if ND4J is to work in OSGi without forcing a choice of CPU/GPU at build time.

@saudet
Copy link
Contributor

saudet commented Aug 23, 2019

Both backends can be built in the same uber JAR, that's not an issue.

If we need to add a dependency on OSGi like that, we'll need to make sure it doesn't break anything anywhere, on Android and Spark among things... Is this something you would have the time to test? :)

@timothyjward
Copy link
Author

If we need to add a dependency on OSGi like that, we'll need to make sure it doesn't break anything anywhere, on Android and Spark among things... Is this something you would have the time to test? :)

I'm happy to help, but I'm not sure I have the appropriate infrastructure available. Unless of course you have solved this problem already?

@saudet
Copy link
Contributor

saudet commented Aug 25, 2019

@timothyjward No, not really unfortunately. It's pretty manual running examples from dl4j-examples for most such integration tests.

@agibsonccc
Copy link
Contributor

@timothyjward @saudet with java 17 LTS forcing module-info I added a semi related PR: #9626

I think I've at least solved part of the problem for modularization. I'm playing with how to add a meta backend that allows 1 or more backends to be present. We also have classloader overriding now. It might allow us to introduce a better way of introducing OSGI support. Any suggestions even if you dont' have time to work on this?

@timothyjward
Copy link
Author

@timothyjward @saudet with java 17 LTS forcing module-info I added a semi related PR: #9626

I think I've at least solved part of the problem for modularization. I'm playing with how to add a meta backend that allows 1 or more backends to be present. We also have classloader overriding now. It might allow us to introduce a better way of introducing OSGI support. Any suggestions even if you dont' have time to work on this?

The biggest issues for OSGi were:

  1. Packages split across multiple jar files.
  2. Highly coupled API packages (API packages recieving/returning types from other packages)
  3. Expectations that you could find implementation classes from the API jar classloader

I'm guessing that item 1 has been solved now as JPMS has a similar requirement. Item 2 has hopefully improved, but isn't necessarily a deal breaker. Item 3 is a big deal, but depending on what the "classloader overriding" support you've added offers it may be sufficient.

I'd suggest adding the bnd-maven-plugin to your build so that you start creating OSGi metadata for each of your jar files. You could then add an OSGi "integration test" project using the bnd-testing-maven-plugin to try some simple scenarios. This will probably not work at first, but will help to figure out:

  1. Problems with resolving (i.e. missing dependencies, unexported API packages)
  2. Problems at runtime (typically expectations of a flat classpath)

These issues can then be worked through one at a time as they are identified until the basic OSGi tests start passing. My guess is that there probably won't be many issues left if the split packages are already gone.

@agibsonccc
Copy link
Contributor

@timothyjward thanks for the comments that helps a lot. I'll try to address what I can in the upgrade.

Regarding 2 and 3:

  1. When splitting things up one problem definitely was the packages things were in. That could stand to be cleaned up. I might do that after releasing the 1.0. For now I'll probably just leave the packages mostly intact and just changing what we need. I agree this should be revisited.

  2. We've added the ability to override everything now with ND4JClassLoading/DL4JClassloading I think with proper injection those issues are mostly solved now.

I'll use this PR for reference and see what I can do about adding proper OSGI metadata. Thanks a lot for your comments.

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.

None yet

3 participants