rzo1 commented on code in PR #704:
URL: https://github.com/apache/opennlp/pull/704#discussion_r1866536436
##########
opennlp-tools/src/main/java/opennlp/tools/util/DownloadUtil.java:
##########
@@ -80,6 +81,41 @@ public enum ModelType {
private static Map<String, Map<ModelType, String>> availableModels;
+ /**
+ * Checks if a model of the specified {@code modelType} has been downloaded
already
+ * for a particular {@code language}.
+ *
+ * @param language The ISO language code of the requested model.
+ * @param modelType The {@link DownloadUtil.ModelType type} of model.
+ * @return {@code true} if a model exists locally, {@code false} otherwise.
+ * @throws IOException Thrown if IO errors occurred or the model is invalid.
+ */
+ static boolean existsModel(String language, ModelType modelType) throws
IOException {
+ Map<ModelType, String> modelsByLanguage =
getAvailableModels().get(language);
+ if (modelsByLanguage == null) {
+ return false;
+ } else {
+ final String url = modelsByLanguage.get(modelType);
+ if (url != null) {
+ final Path homeDirectory =
Paths.get(System.getProperty("OPENNLP_DOWNLOAD_HOME",
+ System.getProperty("user.home"))).resolve(".opennlp");
+ final String filename = url.substring(url.lastIndexOf("/") + 1);
+ final Path localFile = Paths.get(homeDirectory.toString(), filename);
+ boolean exists;
+ try {
Review Comment:
```suggestion
if (Files.exists(localFile)) {
// if this does not throw the requested model exists AND is valid!
validateModel(new URL(url + ".sha512"), localFile);
return true;
} else {
return false;
}
```
Would remove the "control flow by exception" here and do an additional check
via `Files` since we already have the full path available. If it does not
exist, we can fully skip the (hidden) download of a sha512 instead of failing
after the actual sha512 download has happened.
##########
opennlp-tools/src/main/java/opennlp/tools/util/DownloadUtil.java:
##########
@@ -80,6 +81,41 @@ public enum ModelType {
private static Map<String, Map<ModelType, String>> availableModels;
+ /**
+ * Checks if a model of the specified {@code modelType} has been downloaded
already
+ * for a particular {@code language}.
+ *
+ * @param language The ISO language code of the requested model.
+ * @param modelType The {@link DownloadUtil.ModelType type} of model.
+ * @return {@code true} if a model exists locally, {@code false} otherwise.
+ * @throws IOException Thrown if IO errors occurred or the model is invalid.
+ */
+ static boolean existsModel(String language, ModelType modelType) throws
IOException {
+ Map<ModelType, String> modelsByLanguage =
getAvailableModels().get(language);
+ if (modelsByLanguage == null) {
+ return false;
+ } else {
+ final String url = modelsByLanguage.get(modelType);
+ if (url != null) {
+ final Path homeDirectory =
Paths.get(System.getProperty("OPENNLP_DOWNLOAD_HOME",
+ System.getProperty("user.home"))).resolve(".opennlp");
+ final String filename = url.substring(url.lastIndexOf("/") + 1);
+ final Path localFile = Paths.get(homeDirectory.toString(), filename);
Review Comment:
```suggestion
final Path localFile = homeDirectory.resolve(filename);
```
I would avoid the `toString()` operation here and use `resolve(...)`.
##########
opennlp-tools/src/main/java/opennlp/tools/util/DownloadUtil.java:
##########
@@ -80,6 +81,41 @@ public enum ModelType {
private static Map<String, Map<ModelType, String>> availableModels;
+ /**
+ * Checks if a model of the specified {@code modelType} has been downloaded
already
+ * for a particular {@code language}.
+ *
+ * @param language The ISO language code of the requested model.
+ * @param modelType The {@link DownloadUtil.ModelType type} of model.
+ * @return {@code true} if a model exists locally, {@code false} otherwise.
+ * @throws IOException Thrown if IO errors occurred or the model is invalid.
+ */
+ static boolean existsModel(String language, ModelType modelType) throws
IOException {
+ Map<ModelType, String> modelsByLanguage =
getAvailableModels().get(language);
+ if (modelsByLanguage == null) {
+ return false;
+ } else {
+ final String url = modelsByLanguage.get(modelType);
+ if (url != null) {
+ final Path homeDirectory =
Paths.get(System.getProperty("OPENNLP_DOWNLOAD_HOME",
Review Comment:
This is now across the `DownloadUtil`. Maybe we can just extract that into
something like
```java
private static Path getDownloadHome() {
return Paths.get(System.getProperty("OPENNLP_DOWNLOAD_HOME",
System.getProperty("user.home"))).resolve(".opennlp");
}
```
##########
opennlp-tools/src/main/java/opennlp/tools/util/DownloadUtil.java:
##########
@@ -80,6 +81,41 @@ public enum ModelType {
private static Map<String, Map<ModelType, String>> availableModels;
+ /**
+ * Checks if a model of the specified {@code modelType} has been downloaded
already
+ * for a particular {@code language}.
+ *
+ * @param language The ISO language code of the requested model.
+ * @param modelType The {@link DownloadUtil.ModelType type} of model.
+ * @return {@code true} if a model exists locally, {@code false} otherwise.
+ * @throws IOException Thrown if IO errors occurred or the model is invalid.
+ */
+ static boolean existsModel(String language, ModelType modelType) throws
IOException {
+ Map<ModelType, String> modelsByLanguage =
getAvailableModels().get(language);
+ if (modelsByLanguage == null) {
+ return false;
+ } else {
+ final String url = modelsByLanguage.get(modelType);
+ if (url != null) {
+ final Path homeDirectory =
Paths.get(System.getProperty("OPENNLP_DOWNLOAD_HOME",
+ System.getProperty("user.home"))).resolve(".opennlp");
+ final String filename = url.substring(url.lastIndexOf("/") + 1);
+ final Path localFile = Paths.get(homeDirectory.toString(), filename);
+ boolean exists;
+ try {
Review Comment:
Side note: A future enhancement could be to actually download the sha512 to
disk and store it alongside the model files, so we can avoid re-fetching the
sha512 file all the time.
##########
opennlp-tools/src/main/java/opennlp/tools/util/DownloadUtil.java:
##########
@@ -134,17 +170,14 @@ public static <T extends BaseModel> T downloadModel(URL
url, Class<T> type) thro
final Path localFile = Paths.get(homeDirectory.toString(), filename);
if (!Files.exists(localFile)) {
- logger.debug("Downloading model from {} to {}.", url, localFile);
+ logger.debug("Downloading model to {}.", localFile);
try (final InputStream in = url.openStream()) {
Files.copy(in, localFile, StandardCopyOption.REPLACE_EXISTING);
}
-
validateModel(new URL(url + ".sha512"), localFile);
-
logger.debug("Download complete.");
} else {
- System.out.println("Model file already exists. Skipping download.");
Review Comment:
Good catch.
--
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
