Refactoring

[tadejsv]

We need to make sure that all the executors in this repo are rigorously tested, and that the code meets high standards of quality. Here are some things that need to be done:

---

Here is a checklist based on the discussion below

Integration tests

• [ ] Has integration tests
• [ ] Has integration tests with Flow

Unit tests

• [ ] Has unit tests check if docs=None works
• [ ] Has unit tsts check if docs=DocumentArray([])
• [ ] All the modle/files are downloaded/created via fixture
• [ ] All the tmp directories are using tmp_path
• [ ] All the exceptions are tested
• [ ] All the parameters of the public APIs (init() and all the endpoints) are tested.
• [ ] The endpoints are working as expected.

Dockerfile

• [ ] The base image is using FROM jinaai/jina:2-py37-perf

config.yml

• [ ] There is no useless config in config.yml.

README

• [ ] No duplicated information in the README that you can find in the other docs
• [ ] Everything the user needs to know before using this executor is listed in README

Function APIs

• [ ] There is no argument that the user do not need to know but is added.
• [ ] There is no unexpected behavior in the executor.

Model downloading

• [ ] There is no model/files downloaded during init() unless there is a good readon. There is a script for downloding them and there are downloading instructions in the README.

---

[Discussions in Details]

Testing

• each executor needs unit + integration tests
• integration test should consist of sending some documents through a flow with the executor, and checking that the returned documents have expected properties
• unit test should contain
  • checking that all is ok if None is passed for documents
  • checking that all is ok if empty DocumentArray is passed for documents
  • test that loading from config.yaml works (already added)
  • executor specific unit tests (more on that later)
• whenever some model/file needs to be downloaded for testing, this should be done with a pytest fixture before starting tests
• for temporary directories, use tmp_path fixture (tmpdir should be replaced, it is based on the deprecated py library)

Unit tests

On top of the general things mentioned above, unit tests should contain:

• Test usual inputs (make sure that outputs have proper attributes)
• Test that errors/warnings are raised when they should be (same for logs, capture with caplog fixture)
• Test various edge cases
• Test all the parameters in the request(s)
• Quality/semantic test for encoders: take a few simple examples (sentences/images/sound/video), some of which are similar to each other, and make sure that this similarity ranking holds. These should be non-trivial, yet simple examples (e.g. pictures of cat, dog, helicopter and airplane).

Some standards

• let's decide on what the base image should be for all executors
• rename the folder from jinahub to something else
• format of the config.yml should be the same everywhere. Right now, some set empty with and default name field - do we need this or not? In any case, should be the same everywhere
• every single python file needs to be formatted with black, line length 88. We are also using single quotes.
• imports should be sorted (can use isort with black profile for that)
• for readme - I think it makes more sense to put inputs/outputs at the top, right after initialization arguments.

Downloading (large) models

Unless the framework used by the executor uses some convenient option to download the model (like huggingface), the executor should not download any kind of default model. Not during init, not in the dockerfile.

Instead, if the executor is started and the model file does not exist, a very clear error message should be given to the user, explaining to him that he needs to download the model beforehand, and pointing him to the readme (link!) for further instructions.

The readme should contain explicit, clear and copy-pastable instructions on how to download the model (file), plus instructions on what to do if using executor locally (jinahub://), or in a container (jinahub+docker://).

To do

• [x] refactor CI @tadejsv
• [x] refactor text encoders @tadejsv
• [x] #204 @bwanglzu
• [x] refactor video encoders (VideoTorchEncoder) @bwanglzu https://github.com/jina-ai/executors/pull/228
• [x] refactor audio encoders @bwanglzu #223 , #224
• [x] #205 - Tadej
• [x] refactor crafters (Image Normalizer) @JoanFM
• [ ] refactor searchers - Tadej, Bo
• [ ] Move the matchmerger & simpleranker out of monorepo

On hold:

• [ ] refactor storage indexers
• [ ] refactor compound indexers

[CatStark]

Reopened because only CI part is finished

[bwanglzu]

audio encoders: https://github.com/jina-ai/executors/pull/223 https://github.com/jina-ai/executors/pull/224

[nan-wang]

FYI, I've added a checklist for this refactoring. Check out the issue description.

[CatStark]

Closing this and we will have a new one for the Indexers and searchers when the one indexer is ready