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

FEATURE: possibility to define global Classload for ND4J #8972

Merged
merged 1 commit into from Oct 6, 2020

Conversation

hosuaby
Copy link
Contributor

@hosuaby hosuaby commented May 21, 2020

This PR is related to #8712. This issue is talking about the need to have a possibility to provide class loader for method Class.forName. Author mention only class WordVectorSerializer from deeplearning-nlp. But the problem is more general. We need to have possibility to chose classloader used by ND4J and Deeplearning4j.

This PR is a first part of the work. I added a new class ND4JClassLoading that gives possibility to used to provide his own classloader used by Class.forName and ServiceLoader.load. And I replaced all call to those methods by calls to this new class. I hope I forgot nothing.

There will be a second part of this work in anouther PR, concerning Dl4j. Both PR will be inderpendent.

@agibsonccc
Copy link
Contributor

@hosuaby hey sorry about this. I'm just getting to some of these, do you mind updating? I understand if you can't, otherwise we can get this merged.

@agibsonccc
Copy link
Contributor

Hey, sorry this is late. Could you modify it to use the nd4j class loader? Otherwise, this seems legit. Sorry for the late response!

@hosuaby
Copy link
Contributor Author

hosuaby commented Jul 25, 2020

Hello, @agibsonccc :)
Thanks for review. First, I made a rebase of branch and solved conflicts with current master.
I don't understood your demend. If need any modification, don't hesitate to ask. What exactly should I modify ?

@hosuaby
Copy link
Contributor Author

hosuaby commented Jul 26, 2020

Hello, @agibsonccc .

I think I undestood what you wanna say. My mistake, I have better to explain my plan.

This PR is the first part of work concerning #8712. It provides ability to define classloader for ND4J. For DL4J I would like to have a separate class DL4JClassLoading, who will behaivour the same way, but for Dl4j packages.

The reasons I wanna to keep two classloaders separate:

  1. Nd4j and Dl4j are semantically two distinct projects
  2. In my use case, I only need to redefine classloader for ND4j.
  3. If I decide to rely on ND4JClassLoading from dl4j packages, it's will not be clear for user why should he define classloader for nd4j instead of dl4j.

So, my plan is, if this PR is approved, I make immediately the second one with DL4JClassLoading class.

Tell me what you think about.

@agibsonccc agibsonccc self-requested a review July 29, 2020 22:35
@agibsonccc agibsonccc self-assigned this Jul 29, 2020
@hosuaby
Copy link
Contributor Author

hosuaby commented Sep 13, 2020

Hello, @agibsonccc
Any updates about this PR ?

Copy link
Contributor

@agibsonccc agibsonccc left a comment

Choose a reason for hiding this comment

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

Minor nit

@agibsonccc
Copy link
Contributor

@hosuaby minor nit then I think we can merge this. Thanks!

@hosuaby
Copy link
Contributor Author

hosuaby commented Sep 13, 2020

@agibsonccc Ok. Do not hesitate to ask any change!

@agibsonccc
Copy link
Contributor

@hosuaby think you could get this over the line? I think we can merge this soon.

Signed-off-by: hosuaby <alexei.klenin@gmail.com>
@hosuaby
Copy link
Contributor Author

hosuaby commented Oct 5, 2020

@agibsonccc done

@agibsonccc
Copy link
Contributor

Great thanks @hosuaby I want to ping @saudet for 1 review here. @saudet do you see any major issues here?

@saudet
Copy link
Contributor

saudet commented Oct 6, 2020

I didn't test it, but it looks fine.

@agibsonccc agibsonccc merged commit 881a672 into deeplearning4j:master Oct 6, 2020
@hosuaby
Copy link
Contributor Author

hosuaby commented Oct 6, 2020

@agibsonccc Thanks!
Now I will take care of #8712 that is very similar.

agibsonccc pushed a commit to KonduitAI/deeplearning4j that referenced this pull request Oct 13, 2020
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