Accept numpy images #23
Conversation
robotrapta
requested review from
michael-groundlight,
romerocesar and
sunildkumar
robotrapta
force-pushed
the
numpy-images
branch
from
6a4eab5 to
1f148df
Compare
robotrapta
force-pushed
the
numpy-images
branch
from
d8feb76 to
4f18886
Compare
robotrapta
force-pushed
the
numpy-images
branch
from
4c1befc to
5c2c547
Compare
robotrapta
force-pushed
the
numpy-images
branch
from
a711414 to
f2fb3e4
Compare
robotrapta
force-pushed
the
numpy-images
branch
from
5fc2c7a to
ff75013
Compare
|
Okay that was quite a process to get this all built and properly tested. But I think it's in decent shape now. |
| # So the efficiency argument has its limits | ||
| # Failing slow is clearer about what's going on. | ||
| # This is pretty unambiguous, so we're going with it for now. | ||
| fail-fast: false |
There was a problem hiding this comment.
I agree! Diagnosing a failed run quickly is much more important than seeing a failed run quickly.
| else: | ||
| raise TypeError( | ||
| "Unsupported type for image. We only support JPEG images specified through a filename, bytes, BytesIO, or BufferedReader object." | ||
| "Unsupported type for image. We only support numpy arrays (3,W,H) or JPEG images specified through a filename, bytes, BytesIO, or BufferedReader object." |
There was a problem hiding this comment.
is there a world where we expose an error with "np" to the end user? It might be better to be explicit (numpy) everywhere. As someone might not know that np==numpy.
There was a problem hiding this comment.
I think it's pretty universal to import numpy as np.
sunildkumar
left a comment
sunildkumar
left a comment
There was a problem hiding this comment.
lgtm - reducing the number of pain points for a first time user rocks :)
michael-groundlight
left a comment
michael-groundlight
left a comment
There was a problem hiding this comment.
This is definitely a win for customers, even though it's a little funky.
| - name: install numpy | ||
| if: matrix.install_numpy | ||
| run: | | ||
| poetry run pip install numpy | ||
| - name: install pillow | ||
| if: matrix.install_pillow | ||
| run: | | ||
| poetry run pip install pillow |
There was a problem hiding this comment.
I think the poetry-native way to accomplish this would be "optional groups" (In v1.2+ https://python-poetry.org/docs/managing-dependencies/#optional-groups, or "extras" in v1.1: https://python-poetry.org/docs/1.1/pyproject#extras).
Lets you pass in numpy arrays to
submit_image_query. This isn't too hard. But we don't want to require numpy to be installed for it to work. That's also not too hard. But getting idiomatic type-hinting to work when you might or might not have numpy installed - that took some effort. Built some fancy fake-importing to do this. There's a TODO in it to give better error messages, but it all works.Also set up a big test matrix so that we test the SDK with and without numpy, with and without PIL/pillow, and on lots of different versions. Did a good amount of manual verification to ensure that the right tests are being run/skipped in each case.
Fixes #21