search

ufal / udpipe

UDPipe: Trainable pipeline for tokenizing, tagging, lemmatizing and parsing Universal Treebanks and other CoNLL-U files
Mozilla Public License 2.0
358 stars 75 forks source link

Hardcoded static library loading code conflicts with custom library resolving #10

Closed reckart closed 7 years ago

reckart commented 7 years ago

The udpipe_javaJNI contains a static section that tries to load the udpipe library from the current working directory or alternatively using the system-wide library lookup. I want to manually load the library from a custom location. To avoid the built-in library loading code from failing with an exception, it would be good to have an option to disable it.

foxik commented 7 years ago

Do you have any specific suggestion on how to improve the situation? I would like not to require users to explicitly load the library.

BTW, The Java code is generated mostly by Swig (I just added the "try local file first" part).

One possibility for you is to modify the Java wrappers by yourself.

Another possibility is that the static constructor could maybe fail silently, and the module code (in the Swig terminology, i.e. cz.cuni.mff.ufal.udpipe.udpipe_java) would contain two methods -- load_udpipe_library and is_udpipe_library_loaded. So if the library is found in the static constructor, it would be used, but you could provide another path before calling any method from the JNI in case -- but you would not be able to provide the override if the library is found in the static constructor.

Maybe you would prefer version without the static constructor entirely? We maybe coud have two versions of the bindings.

The "ideal" solution would probably be to load the JNI library only after the first call into it, so that you could specify override paths before any JNI loading -- but I am unsure how to achieve this with Swig.

Ideas welcome :-)

reckart commented 7 years ago

I checked an older swig-based interface for Mecab that we are using - it does not have this static auto-load code. We load the library ourselves.

The "ideal" solution would probably be to load the JNI library only after the first call into it, so that you could specify override paths before any JNI loading -- but I am unsure how to achieve this with Swig.

Yep ;)

Another possibility is that the static constructor could maybe fail silently, and the module code (in the Swig terminology, i.e. cz.cuni.mff.ufal.udpipe.udpipe_java) would contain two methods -- load_udpipe_library and is_udpipe_library_loaded.

I think those two additional methods would be great. But I don't think the static constructor should silently fail in that case. If is_udpipe_library_loaded returns true, it should do nothing. Otherwise, it should try to auto-load and fail if it does not work. Why should it fail silently?

P.S.: Do you know JavaCPP? I've seen it being used e.g. in DL4J. Seems like a nice way of packaging up natively compiled libraries along with their Java interfaces in JARs and to distribute them via Maven. Not used it myself though.

foxik commented 7 years ago

I think those two additional methods would be great. But I don't think the static constructor should silently fail in that case. If is_udpipe_library_loaded returns true, it should do nothing. Otherwise, it should try to auto-load and fail if it does not work. Why should it fail silently?

I am not that good in Java, I thought the static constructor would be called too soon (right after the import). But after some experiments it seems that the udpipe_javaJNI static constuctor is called only after the first call to any static method of udpipe_javaJNI. Therefore, the "ideal" solution cited above is easy to implement -- udpipe_java could contain a String udpipe_library_path (with default set to library in the current directory), which could be overriden using udpipe_java.set_udpipe_library_path, and the udpipe_javaJNI static constructor would try loading the library from that path.

Sounds good?

PS: No, I do not know JavaCPP. We are using SWIG for Python, Perl and C#, so I was happy using it for Java too.

reckart commented 7 years ago

Sounds good.

foxik commented 7 years ago

It seems to be working.

PS: Sorry for the forced update.

reckart commented 7 years ago

I wonder if you consider doing another release with this change included any time soon?

foxik commented 7 years ago

There will be a release in several weeks (2-3), containing this change (and several others, which are not yet fully implemented).

reckart commented 7 years ago

Cool! Looking forward to it!