search

jina-ai / executors

internal-only
Apache License 2.0
31 stars 12 forks source link

feat(executor): Add Timm image encoder #153

Closed vivek2301 closed 2 years ago

vivek2301 commented 3 years ago

closes #136

vivek2301 commented 3 years ago

@tadejsv If I remove Dockerfile then should I remove "usage via docker image" section from README too? It was added by default when I created the encoder using jinahub.

tadejsv commented 3 years ago

@vivek2301 no, we automatically generate the Dockerfile in CI and for use with jinahub and jinahub+docker

vivek2301 commented 3 years ago

@tadejsv,

I made the suggested changes and added tests. Can you please review it?

vivek2301 commented 3 years ago

@tadejsv I've fixed the lint error with isort. One test failed because the test couldn't pull docker image timmimageencoder. According to contribution guideline, this test needs to be included. I don't see any step in CICD creating this image but according to the documentation there should be one. The second test fail is in outer integration test test_dump_psql.py. I did not make any changes in this.

Any hints on what should I do to fix these?

tadejsv commented 3 years ago

@vivek2301 apologies for not reviewing yet - today is my day off, I will look at it first thing on monday

vivek2301 commented 3 years ago

No problem, Tadej. Just an update - I noticed that the documentation was updated recently for docker test case and I updated the test accordingly. It should fix the test case.

vivek2301 commented 3 years ago

Thanks for the review @tadejsv, I will update the PR accordingly tomorrow.

vivek2301 commented 3 years ago

@tadejsv I made the suggested changes. Also, I think similar changes are needed for ImageTorchEncoder too. I followed Torch encoder to build Timm encoder and it also has similar issues.

cristianmtr commented 3 years ago

@vivek2301 @tadejsv what's the status here?

tadejsv commented 3 years ago

@vivek2301 seems all my comments were adressed, right?

I see that there is a failure in CI, but that's due to external reasons, I'll investigate

vivek2301 commented 3 years ago

@tadejsv That's correct, I integrated all your comments.

tadejsv commented 3 years ago

@vivek2301, can you merge in the main branch? The recent changes on it should allow the CI to pass, so that we can merge this PR

vivek2301 commented 3 years ago

@tadejsv I've merged with the main branch and resolved the comments. Can you please run the workflow?