rzo1 commented on PR #786: URL: https://github.com/apache/opennlp/pull/786#issuecomment-2932019361
Great things are happening here. Thanks for the push forward @mawiesne I have some questions / thoughts / points for further discussion: - What is the (planned) content to be put under `opennlp-ml`? I think it aims to carve out the maxent parts (or everything under `ml` package? `opennlp-dl` and `opennlp-dl-gpu` are technically also part of machine learning. Maybe we can put up `opennlp-ml` as a pom module and add `opennlp-maxent` and the dl stuff as children under it? - Do we (still) aim for (package) compatibility or should we use this major version to align the package imports with ASF standards and move to `org.apache.opennlp` ? - I think we need to double check the provided `package-info.java` for JPMS (or drop JPMS metadata doc support because our current multi maven structure with split packages would result in JPMS errors anyway). At the moment, it looks a bit inconsistent (but is also a thing for 2.x, I guess) because package-info.java files are meant for metadata, having them in multiple modules defining the same package can confuse tools (like static analyzers, etc), even outside JPMS. ```bash ➜ opennlp git:(OPENNLP-1708-Introduce-new-module-structure) find . -name package-info.java ./opennlp-api/src/main/java/opennlp/tools/commons/package-info.java ./opennlp-core/opennlp-formats/src/main/java/opennlp/tools/formats/muc/package-info.java ./opennlp-core/opennlp-formats/src/main/java/opennlp/tools/formats/ad/package-info.java ./opennlp-core/opennlp-formats/src/main/java/opennlp/tools/formats/frenchtreebank/package-info.java ./opennlp-core/opennlp-formats/src/main/java/opennlp/tools/formats/masc/package-info.java ./opennlp-core/opennlp-formats/src/main/java/opennlp/tools/formats/brat/package-info.java ./opennlp-core/opennlp-formats/src/main/java/opennlp/tools/formats/leipzig/package-info.java ./opennlp-core/opennlp-formats/src/main/java/opennlp/tools/formats/conllu/package-info.java ./opennlp-core/opennlp-formats/src/main/java/opennlp/tools/formats/nkjp/package-info.java ./opennlp-core/opennlp-formats/src/main/java/opennlp/tools/formats/letsmt/package-info.java ./opennlp-core/opennlp-formats/src/main/java/opennlp/tools/formats/irishsentencebank/package-info.java ./opennlp-core/opennlp-formats/src/main/java/opennlp/tools/formats/ontonotes/package-info.java ./opennlp-core/opennlp-formats/src/main/java/opennlp/tools/formats/package-info.java ./opennlp-core/opennlp-formats/src/main/java/opennlp/tools/package-info.java ./opennlp-core/opennlp-cli/src/main/java/opennlp/tools/cmdline/lemmatizer/package-info.java ./opennlp-core/opennlp-cli/src/main/java/opennlp/tools/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/lemmatizer/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/ext/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/featuregen/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/namefind/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/entitylinker/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/languagemodel/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/parser/chunking/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/parser/treeinsert/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/parser/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/ngram/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/postag/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/ml/naivebayes/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/ml/maxent/io/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/ml/maxent/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/ml/maxent/quasinewton/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/ml/perceptron/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/ml/model/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/ml/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/dictionary/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/log/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/doccat/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/sentdetect/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/chunker/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/langdetect/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/tokenize/package-info.java ./opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/package-info.java ./opennlp-tools/src/main/java/opennlp/tools/util/package-info.java ./opennlp-tools/src/main/java/opennlp/tools/package-info.java ``` # Other Notes - Build did succeed on my machine. Checked the diff in IDE + the new classes -> lgtm. - If we have more input from the others, I would suggest to just make the move and merge the big changes into `main` and do the rest of the work "step-by-step" in iteration mode via smaller PRs to reduce review burden :) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@opennlp.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
