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

NearestNeighboursServer logging improvements #4272

Merged
merged 3 commits into from Nov 13, 2017

Conversation

@AlexDBlack
Copy link
Contributor

commented Nov 9, 2017

No description provided.

@AlexDBlack AlexDBlack requested a review from agibsonccc Nov 9, 2017

@huitseeker
Copy link

left a comment

The logging LGTM.
There's already two copies of this exact file in the repo:

huitseeker@pegasus➜/tmp» sha1sum application.conf                    [18:31:22]
cd1b4fbc109eb9bb443719e196fc9ff9608988dc  application.conf
huitseeker@pegasus➜/tmp» cd ~/DL4J/deeplearning4j                    [18:31:34]
huitseeker@pegasus➜huitseeker/DL4J/deeplearning4j(master)» find ./ -iname application.conf                                                                     
./deeplearning4j-ui-parent/deeplearning4j-play/target/classes/application.conf
./deeplearning4j-ui-parent/deeplearning4j-play/src/main/resources/application.conf
huitseeker@pegasus➜huitseeker/DL4J/deeplearning4j(master)» sha1sum ./deeplearning4j-ui-parent/deeplearning4j-play/target/classes/application.conf
cd1b4fbc109eb9bb443719e196fc9ff9608988dc  ./deeplearning4j-ui-parent/deeplearning4j-play/target/classes/application.conf
huitseeker@pegasus➜huitseeker/DL4J/deeplearning4j(master)» sha1sum ./deeplearning4j-ui-parent/deeplearning4j-play/src/main/resources/application.conf
cd1b4fbc109eb9bb443719e196fc9ff9608988dc  ./deeplearning4j-ui-parent/deeplearning4j-play/src/main/resources/application.conf

And it really doesn't have a lot of uncommented content:
https://gist.github.com/24e0e9e6b63c2ecb253d995ac88164b4

Is there any way we could point the server test to another application.conf with something like

URL url = Thread.currentThread().getContextClassLoader().getResource("mypackage/bla/bla/application.conf");
File file = new File(url.getPath());

Or at least make this file minimal, to clearly communicate this is a test stub?

@AlexDBlack

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2017

Right, that's a fair point. It's probably possible using relative paths or something... Though I guess if we are doing that - we might as well do it with logging configuration (logback.xml) too?

@huitseeker

This comment has been minimized.

Copy link

commented Nov 10, 2017

As far as I'm concerned, auxiliary files (applicaiton,conf, logback ...):

  • can arguably be duplicated in the main,
  • should be minimized to stubs or relative in the test.
@AlexDBlack

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2017

Hm, though if application.conf and logback.xml for test resources are defined in relative terms, they won't be on the classpath (which, alone, won't be sufficient). I agree with the motivation to reduce duplication though... I think our best option might be to:

(a) Have a single copy in say deeplearning4j-core or -nn (unless somewhere else would be better?)

(b) Configure maven surefure plugin to set relevant system properties for each library that requires them
http://maven.apache.org/surefire/maven-surefire-plugin/examples/system-properties.html
Play: -Dconfig.file=
Logback: -Dlogback.configurationFile=

https://www.playframework.com/documentation/2.4.x/ProductionConfiguration#Specifying-an-alternate-configuration-file
https://logback.qos.ch/manual/configuration.html

@AlexDBlack AlexDBlack force-pushed the ab_knn branch from bf9bf91 to 4bfdf13 Nov 13, 2017

@AlexDBlack AlexDBlack merged commit d70537e into master Nov 13, 2017

@AlexDBlack

This comment has been minimized.

Copy link
Contributor Author

commented Nov 13, 2017

@huitseeker thanks for the review, I've changed it to use the existing one instead (via system property). Seems to be finding it ok using that approach.

For logback.xml: I've opened this issue #4288

@AlexDBlack AlexDBlack deleted the ab_knn branch Nov 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.