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

Fix for TIKA-2400 Standardizing current Object Recognition REST parsers #208

Merged
merged 18 commits into from Nov 21, 2017

Conversation

Projects
None yet
4 participants
@ThejanW
Member

ThejanW commented Sep 17, 2017

This PR consists of,

  1. Reformatting related to inceptionapi.py, im2txtapi.py and related Java clients
  2. Logic implementations for checking min confidence in server side instead of client side in Object Recognition REST parsers
  3. Refactoring to docker files

How to test?

  1. docker run -it -p 8764:8764 uscdatascience/inception-rest-tika - then run the tests in ObjectRecognitionParserTest class

  2. docker run -it -p 8764:8764 uscdatascience/im2txt-rest-tika - then run the tests in ObjectRecognitionParserTest class

  3. docker run -it -p 8764:8764 uscdatascience/inception-video-rest-tika - then run the tests in TensorflowVideoRecParserTest class

ThejanW added some commits Sep 16, 2017

TIKA 2400 :
# Define apiBaseUri for inceptionREST
# Link wiki pages
TIKA 2400 :
# Fix formatting issues of inceptionapi.py
# include logic for checking minimum confidence
TIKA 2400 : Changes to video_util.py
# Fix formatting issues
# Remove unused imports
TIKA 2400 :
# Adjust the Object Recognition REST clients to work with changed servers
TIKA 2400 :
# Few refactoring to Object Recognition REST clients
@thammegowda

This comment has been minimized.

Show comment
Hide comment
@thammegowda

thammegowda Sep 18, 2017

Contributor

@ThejanW Great work. Looks like you have done lot of cleaning, so here is another 👍

Please publish these docker images under some organization. Since we cannot use apache organization under docker hub, lets just use https://hub.docker.com/u/uscdatascience/, I gave you all the permissions.

Contributor

thammegowda commented Sep 18, 2017

@ThejanW Great work. Looks like you have done lot of cleaning, so here is another 👍

Please publish these docker images under some organization. Since we cannot use apache organization under docker hub, lets just use https://hub.docker.com/u/uscdatascience/, I gave you all the permissions.

@thammegowda thammegowda self-requested a review Sep 18, 2017

@chrismattmann

This comment has been minimized.

Show comment
Hide comment
@chrismattmann

chrismattmann Sep 18, 2017

Contributor

Yes please use @uscdataacience thanks dudes

Contributor

chrismattmann commented Sep 18, 2017

Yes please use @uscdataacience thanks dudes

@ThejanW

This comment has been minimized.

Show comment
Hide comment
@ThejanW

ThejanW Sep 18, 2017

Member

@chrismattmann @thammegowda yeah! lemme configure docker builds in uscdatascience 👍

Member

ThejanW commented Sep 18, 2017

@chrismattmann @thammegowda yeah! lemme configure docker builds in uscdatascience 👍

@@ -83,12 +83,6 @@ public int compare(RecognisedObject o1, RecognisedObject o2) {
}
};
@Field
private double minConfidence = 0.05;

This comment has been minimized.

@smadha

smadha Sep 20, 2017

Contributor
  • Any specific reason to remove minConfidence and topN ?
@smadha

smadha Sep 20, 2017

Contributor
  • Any specific reason to remove minConfidence and topN ?

This comment has been minimized.

@smadha

smadha Sep 20, 2017

Contributor

If you plan to put these controls in REST URI then please leave it somewhere in comments and wiki too. Also, this needs to be updated in comments too - https://github.com/ThejanW/tika/blob/92c65e0a43e7f09a0566bec34f352314dffe5def/tika-parsers/src/main/java/org/apache/tika/parser/recognition/ObjectRecognitionParser.java#L60

@smadha

smadha Sep 20, 2017

Contributor

If you plan to put these controls in REST URI then please leave it somewhere in comments and wiki too. Also, this needs to be updated in comments too - https://github.com/ThejanW/tika/blob/92c65e0a43e7f09a0566bec34f352314dffe5def/tika-parsers/src/main/java/org/apache/tika/parser/recognition/ObjectRecognitionParser.java#L60

This comment has been minimized.

@ThejanW

ThejanW Sep 20, 2017

Member

Yeah, I have moved minConfidence logic to REST servers, it is kind of odd to ask for topk objects from the backend and filter those objects again in the client with related to minConfidence and select topN objects, just too much logic in the client. we can directly ask the backend to give us topN objects which has a confidence greater than the minConfidence, less iterations and simplified client 💯

@ThejanW

ThejanW Sep 20, 2017

Member

Yeah, I have moved minConfidence logic to REST servers, it is kind of odd to ask for topk objects from the backend and filter those objects again in the client with related to minConfidence and select topN objects, just too much logic in the client. we can directly ask the backend to give us topN objects which has a confidence greater than the minConfidence, less iterations and simplified client 💯

This comment has been minimized.

@ThejanW

ThejanW Sep 20, 2017

Member

Hey! good catch..it's not easy maintaining comments like these(https://github.com/ThejanW/tika/blob/92c65e0a43e7f09a0566bec34f352314dffe5def/tika-parsers/src/main/java/org/apache/tika/parser/recognition/ObjectRecognitionParser.java#L49-L70)

A future developers will also miss these. Will update them asap 👍

@ThejanW

ThejanW Sep 20, 2017

Member

Hey! good catch..it's not easy maintaining comments like these(https://github.com/ThejanW/tika/blob/92c65e0a43e7f09a0566bec34f352314dffe5def/tika-parsers/src/main/java/org/apache/tika/parser/recognition/ObjectRecognitionParser.java#L49-L70)

A future developers will also miss these. Will update them asap 👍

This comment has been minimized.

@smadha

smadha Sep 22, 2017

Contributor

@ThejanW For my understanding minConfidence and topN can still be tweaked through Tika config / CLI options right?

@smadha

smadha Sep 22, 2017

Contributor

@ThejanW For my understanding minConfidence and topN can still be tweaked through Tika config / CLI options right?

This comment has been minimized.

@ThejanW

ThejanW Sep 22, 2017

Member

sorry, I misunderstood your question, the reason why I have removed minConfidence and topN from objectRecognitionParser is, objectRecognitionParser does not need to keep such client specific parameters. Those client specific fields should be in that specific client, we are just using ObjectRecognitionParser to process objects from the respective REST client and put them in the xhtml.

@ThejanW

ThejanW Sep 22, 2017

Member

sorry, I misunderstood your question, the reason why I have removed minConfidence and topN from objectRecognitionParser is, objectRecognitionParser does not need to keep such client specific parameters. Those client specific fields should be in that specific client, we are just using ObjectRecognitionParser to process objects from the respective REST client and put them in the xhtml.

This comment has been minimized.

@ThejanW

ThejanW Sep 22, 2017

Member

yes, minConfidence and topN can be set through CLI/ Tika Config since we have defined them in REST clients. In TensorflowRESTVideoRecogniser, you're extending TensorflowRESTRecogniser, that's why I have made some of the fields in TensorflowRESTRecogniser as protected(we need them there to derive apiUri and healthUri).

@ThejanW

ThejanW Sep 22, 2017

Member

yes, minConfidence and topN can be set through CLI/ Tika Config since we have defined them in REST clients. In TensorflowRESTVideoRecogniser, you're extending TensorflowRESTRecogniser, that's why I have made some of the fields in TensorflowRESTRecogniser as protected(we need them there to derive apiUri and healthUri).

This comment has been minimized.

@thammegowda

thammegowda Sep 24, 2017

Contributor

Correct me if my understanding is wrong:

  • we have removed minConfidence and topN from ObjectRecognitionParser
  • We have added them to classes that implement ObjectRecogniser interface - Like TensorflowRestRecogniser, TensforflowRestImageCaptioner etc .. These are referred as client in Thejan's terminology
  • We also have URL accompanying each client, which allow tweaking of these parameters.

Food for Design thought: We might not have URLs for every client. to be specific - we could have a client using DL4J that doesn't use REST communication. So these parameters are required for the client and hence they should have it.

@thammegowda

thammegowda Sep 24, 2017

Contributor

Correct me if my understanding is wrong:

  • we have removed minConfidence and topN from ObjectRecognitionParser
  • We have added them to classes that implement ObjectRecogniser interface - Like TensorflowRestRecogniser, TensforflowRestImageCaptioner etc .. These are referred as client in Thejan's terminology
  • We also have URL accompanying each client, which allow tweaking of these parameters.

Food for Design thought: We might not have URLs for every client. to be specific - we could have a client using DL4J that doesn't use REST communication. So these parameters are required for the client and hence they should have it.

This comment has been minimized.

@ThejanW

ThejanW Sep 30, 2017

Member

@thammegowda your understanding is exactly correct.

@ThejanW

ThejanW Sep 30, 2017

Member

@thammegowda your understanding is exactly correct.

Show outdated Hide outdated ...ain/java/org/apache/tika/parser/recognition/ObjectRecognitionParser.java
Show outdated Hide outdated ...ain/java/org/apache/tika/parser/recognition/ObjectRecognitionParser.java
Show outdated Hide outdated ...ain/java/org/apache/tika/parser/recognition/ObjectRecognitionParser.java
@@ -56,7 +57,7 @@
* Tensor Flow image recogniser which has high performance.
* This implementation uses Tensorflow via REST API.
* <p>
* NOTE : //TODO: link to wiki page here
* NOTE : https://wiki.apache.org/tika/TikaAndVision

This comment has been minimized.

@smadha

smadha Sep 20, 2017

Contributor

Thanks

@smadha

smadha Sep 20, 2017

Contributor

Thanks

@@ -160,4 +175,4 @@ public void checkInitialization(InitializableProblemHandler handler)
LOG.debug("Num Objects found {}", recObjs.size());
return recObjs;
}
}
}

This comment has been minimized.

@smadha

smadha Sep 20, 2017

Contributor
  • Extra line break
@smadha

smadha Sep 20, 2017

Contributor
  • Extra line break
/**
* Tensor Flow video recogniser which has high performance.
* This implementation uses Tensorflow via REST API.
* <p>
* NOTE : //TODO: link to wiki page here
* NOTE : https://wiki.apache.org/tika/TikaAndVisionVideo

This comment has been minimized.

@smadha

smadha Sep 20, 2017

Contributor

👍

@smadha

smadha Sep 20, 2017

Contributor

👍

*
* @since Apache Tika 1.15
*/
public class TensorflowRESTVideoRecogniser extends TensorflowRESTRecogniser{
public class TensorflowRESTVideoRecogniser extends TensorflowRESTRecogniser {

This comment has been minimized.

@smadha

smadha Sep 20, 2017

Contributor
  • Extra space
@smadha

smadha Sep 20, 2017

Contributor
  • Extra space
private static final Logger LOG = LoggerFactory.getLogger(TensorflowRESTRecogniser.class);
private static final Logger LOG = LoggerFactory.getLogger(TensorflowRESTVideoRecogniser.class);

This comment has been minimized.

@smadha

smadha Sep 20, 2017

Contributor

Super thanks

@smadha

smadha Sep 20, 2017

Contributor

Super thanks

@@ -1,5 +1,5 @@
#!/usr/bin/env python
#

This comment has been minimized.

@smadha

smadha Sep 20, 2017

Contributor

I guess there are very few actual changes in this file but mostly extra spaces and new lines. Though your code is great I'll suggest few of extra spaces and new lines in future as it brings focus to actual change only. Makes sense?

@smadha

smadha Sep 20, 2017

Contributor

I guess there are very few actual changes in this file but mostly extra spaces and new lines. Though your code is great I'll suggest few of extra spaces and new lines in future as it brings focus to actual change only. Makes sense?

@thammegowda

thammegowda requested changes Sep 24, 2017 edited

@ThejanW Thanks again for cleaning the code that evolved over past two years!
I made some suggestions.
Also, I cant test video docker

docker run -it -p 8764:8764 uscdatascience/inception-rest-tika
Can't import video libraries, No video functionality is available
Traceback (most recent call last):
  File "/usr/bin/inceptionapi.py", line 265, in <module>
    app = Classifier(__name__)
  File "/usr/bin/inceptionapi.py", line 221, in __init__
    self.names = imagenet.create_readable_names_for_imagenet_labels()
  File "/models-c15fada28113eca32dc98d6e3bec4755d0d5b4c2/slim/datasets/imagenet.py", line 93, in create_readable_names_for_imagenet_labels
    assert num_synsets_in_ilsvrc == 1000
AssertionError

I did docker rmi -f for previous images but stil something is wrong.

Show outdated Hide outdated ...n/java/org/apache/tika/parser/captioning/tf/TensorflowRESTCaptioner.java
@@ -107,7 +107,7 @@ public boolean isAvailable() {
public void initialize(Map<String, Param> params) throws TikaConfigException {
try {
healthUri = URI.create(apiBaseUri + "/ping");
apiUri = URI.create(apiBaseUri + String.format(Locale.getDefault(), "/captions?beam_size=%1$d&max_caption_length=%2$d",
apiUri = URI.create(apiBaseUri + String.format(Locale.getDefault(), "/caption/image?beam_size=%1$d&max_caption_length=%2$d",

This comment has been minimized.

@thammegowda

thammegowda Sep 24, 2017

Contributor

Improvement: String.format(Locale.getDefault(), ...) and String.format(...) are equivalent right (default is inferred implicitely)?
Rule of thumb - 1) When you have two options, pick the simple one! For me, latter one looks simple
2) If you want to enforce a specific locale, then it not same as default.

@thammegowda

thammegowda Sep 24, 2017

Contributor

Improvement: String.format(Locale.getDefault(), ...) and String.format(...) are equivalent right (default is inferred implicitely)?
Rule of thumb - 1) When you have two options, pick the simple one! For me, latter one looks simple
2) If you want to enforce a specific locale, then it not same as default.

@@ -83,12 +83,6 @@ public int compare(RecognisedObject o1, RecognisedObject o2) {
}
};
@Field
private double minConfidence = 0.05;

This comment has been minimized.

@thammegowda

thammegowda Sep 24, 2017

Contributor

Correct me if my understanding is wrong:

  • we have removed minConfidence and topN from ObjectRecognitionParser
  • We have added them to classes that implement ObjectRecogniser interface - Like TensorflowRestRecogniser, TensforflowRestImageCaptioner etc .. These are referred as client in Thejan's terminology
  • We also have URL accompanying each client, which allow tweaking of these parameters.

Food for Design thought: We might not have URLs for every client. to be specific - we could have a client using DL4J that doesn't use REST communication. So these parameters are required for the client and hence they should have it.

@thammegowda

thammegowda Sep 24, 2017

Contributor

Correct me if my understanding is wrong:

  • we have removed minConfidence and topN from ObjectRecognitionParser
  • We have added them to classes that implement ObjectRecogniser interface - Like TensorflowRestRecogniser, TensforflowRestImageCaptioner etc .. These are referred as client in Thejan's terminology
  • We also have URL accompanying each client, which allow tweaking of these parameters.

Food for Design thought: We might not have URLs for every client. to be specific - we could have a client using DL4J that doesn't use REST communication. So these parameters are required for the client and hence they should have it.

Show outdated Hide outdated ...ain/java/org/apache/tika/parser/recognition/ObjectRecognitionParser.java
Show outdated Hide outdated ...ain/java/org/apache/tika/parser/recognition/ObjectRecognitionParser.java
Show outdated Hide outdated ...java/org/apache/tika/parser/recognition/tf/TensorflowRESTRecogniser.java
@ThejanW

This comment has been minimized.

Show comment
Hide comment
@ThejanW

ThejanW Sep 30, 2017

Member

@thammegowda you can't test the video docker, because you haven't pulled the correct docker image. The docker image for video docker is uscdatascience/inception-video-rest-tika. Please see my initial comment of this PR.

Member

ThejanW commented Sep 30, 2017

@thammegowda you can't test the video docker, because you haven't pulled the correct docker image. The docker image for video docker is uscdatascience/inception-video-rest-tika. Please see my initial comment of this PR.

@thammegowda

This comment has been minimized.

Show comment
Hide comment
@thammegowda

thammegowda Oct 15, 2017

Contributor

I cant test image as well as video docker.

docker run -it -p 8764:8764 uscdatascience/inception-rest-tika
Unable to find image 'uscdatascience/inception-rest-tika:latest' locally
latest: Pulling from uscdatascience/inception-rest-tika
9fb6c798fa41: Already exists
3b61febd4aef: Already exists
9d99b9777eb0: Already exists
d010c8cf75d7: Already exists
7fac07fb303e: Already exists
5601f0fca79b: Already exists
dad2688af054: Already exists
efa7176a3f6c: Already exists
5ba941a90099: Already exists
b5a6f1155f94: Already exists
7e863f718dc4: Already exists
Digest: sha256:20840a9c9e5cd2fed7d6c19ba322228901c9ba6ec06fe0afe13b9f6624dc12e2
Status: Downloaded newer image for uscdatascience/inception-rest-tika:latest
Can't import video libraries, No video functionality is available
Traceback (most recent call last):
  File "/usr/bin/inceptionapi.py", line 265, in <module>
    app = Classifier(__name__)
  File "/usr/bin/inceptionapi.py", line 221, in __init__
    self.names = imagenet.create_readable_names_for_imagenet_labels()
  File "/models-c15fada28113eca32dc98d6e3bec4755d0d5b4c2/slim/datasets/imagenet.py", line 93, in create_readable_names_for_imagenet_labels
    assert num_synsets_in_ilsvrc == 1000
AssertionError
$ docker run -it -p 8764:8764 uscdatascience/inception-video-rest-tika
.........
cv2.__version__ 3.2.0
Traceback (most recent call last):
  File "/usr/bin/inceptionapi.py", line 265, in <module>
    app = Classifier(__name__)
  File "/usr/bin/inceptionapi.py", line 221, in __init__
    self.names = imagenet.create_readable_names_for_imagenet_labels()
  File "/models-c15fada28113eca32dc98d6e3bec4755d0d5b4c2/slim/datasets/imagenet.py", line 93, in create_readable_names_for_imagenet_labels
    assert num_synsets_in_ilsvrc == 1000
AssertionError

I will wait for others to test and confirm if the issue is with my docker setup or with the images

Contributor

thammegowda commented Oct 15, 2017

I cant test image as well as video docker.

docker run -it -p 8764:8764 uscdatascience/inception-rest-tika
Unable to find image 'uscdatascience/inception-rest-tika:latest' locally
latest: Pulling from uscdatascience/inception-rest-tika
9fb6c798fa41: Already exists
3b61febd4aef: Already exists
9d99b9777eb0: Already exists
d010c8cf75d7: Already exists
7fac07fb303e: Already exists
5601f0fca79b: Already exists
dad2688af054: Already exists
efa7176a3f6c: Already exists
5ba941a90099: Already exists
b5a6f1155f94: Already exists
7e863f718dc4: Already exists
Digest: sha256:20840a9c9e5cd2fed7d6c19ba322228901c9ba6ec06fe0afe13b9f6624dc12e2
Status: Downloaded newer image for uscdatascience/inception-rest-tika:latest
Can't import video libraries, No video functionality is available
Traceback (most recent call last):
  File "/usr/bin/inceptionapi.py", line 265, in <module>
    app = Classifier(__name__)
  File "/usr/bin/inceptionapi.py", line 221, in __init__
    self.names = imagenet.create_readable_names_for_imagenet_labels()
  File "/models-c15fada28113eca32dc98d6e3bec4755d0d5b4c2/slim/datasets/imagenet.py", line 93, in create_readable_names_for_imagenet_labels
    assert num_synsets_in_ilsvrc == 1000
AssertionError
$ docker run -it -p 8764:8764 uscdatascience/inception-video-rest-tika
.........
cv2.__version__ 3.2.0
Traceback (most recent call last):
  File "/usr/bin/inceptionapi.py", line 265, in <module>
    app = Classifier(__name__)
  File "/usr/bin/inceptionapi.py", line 221, in __init__
    self.names = imagenet.create_readable_names_for_imagenet_labels()
  File "/models-c15fada28113eca32dc98d6e3bec4755d0d5b4c2/slim/datasets/imagenet.py", line 93, in create_readable_names_for_imagenet_labels
    assert num_synsets_in_ilsvrc == 1000
AssertionError

I will wait for others to test and confirm if the issue is with my docker setup or with the images

@ThejanW

This comment has been minimized.

Show comment
Hide comment
@ThejanW

ThejanW Oct 16, 2017

Member

I was getting the same error. Nothing is wrong with your docker setup. The problem was with the download url of imagenet_lsvrc_2015_synsets.txt & imagenet_metadata.txt. Apparently tf maintainers have moved these meta files and models to another repo https://github.com/tensorflow/serving.
See, https://raw.githubusercontent.com/tensorflow/models/master/inception/inception/data/imagenet_lsvrc_2015_synsets.txt
https://raw.githubusercontent.com/tensorflow/models/master/inception/inception/data/imagenet_metadata.txt
you will get 404. I'll update with the new URLs

Member

ThejanW commented Oct 16, 2017

I was getting the same error. Nothing is wrong with your docker setup. The problem was with the download url of imagenet_lsvrc_2015_synsets.txt & imagenet_metadata.txt. Apparently tf maintainers have moved these meta files and models to another repo https://github.com/tensorflow/serving.
See, https://raw.githubusercontent.com/tensorflow/models/master/inception/inception/data/imagenet_lsvrc_2015_synsets.txt
https://raw.githubusercontent.com/tensorflow/models/master/inception/inception/data/imagenet_metadata.txt
you will get 404. I'll update with the new URLs

@ThejanW

This comment has been minimized.

Show comment
Hide comment
@ThejanW

ThejanW Nov 6, 2017

Member

@thammegowda @chrismattmann @smadha This is complete now. I have updated tensorflow version and models to the latest(tf 1.4.0). Currently object rec REST parsers are not functioning due to the URL change of imagenet_lsvrc_2015_synsets.txt & imagenet_metadata.txt. By this PR, those issues can also be resolved. Therefore it would be nice if we can merge this before 1.17. Testing instructions are included in the initial comment.

Member

ThejanW commented Nov 6, 2017

@thammegowda @chrismattmann @smadha This is complete now. I have updated tensorflow version and models to the latest(tf 1.4.0). Currently object rec REST parsers are not functioning due to the URL change of imagenet_lsvrc_2015_synsets.txt & imagenet_metadata.txt. By this PR, those issues can also be resolved. Therefore it would be nice if we can merge this before 1.17. Testing instructions are included in the initial comment.

@ThejanW

This comment has been minimized.

Show comment
Hide comment
@ThejanW

ThejanW Nov 21, 2017

Member

@chrismattmann can we merge this?

Member

ThejanW commented Nov 21, 2017

@chrismattmann can we merge this?

@chrismattmann chrismattmann merged commit 91ef9a9 into apache:master Nov 21, 2017

@chrismattmann

This comment has been minimized.

Show comment
Hide comment
@chrismattmann

chrismattmann Nov 21, 2017

Contributor

yes, looks great! great job @ThejanW

Contributor

chrismattmann commented Nov 21, 2017

yes, looks great! great job @ThejanW

@chrismattmann

This comment has been minimized.

Show comment
Hide comment
@chrismattmann

chrismattmann Nov 21, 2017

Contributor

@ThejanW can you please also remove the Docker files present in captioning/tf and in recognition/tf?

Contributor

chrismattmann commented Nov 21, 2017

@ThejanW can you please also remove the Docker files present in captioning/tf and in recognition/tf?

@chrismattmann

This comment has been minimized.

Show comment
Hide comment
@chrismattmann

chrismattmann Nov 21, 2017

Contributor

nevermind @ThejanW I did it

Contributor

chrismattmann commented Nov 21, 2017

nevermind @ThejanW I did it

chrismattmann added a commit that referenced this pull request Nov 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment