Skip to content
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

feat(python): design an image extension type #1272

Merged
merged 22 commits into from
Oct 10, 2023
Merged

feat(python): design an image extension type #1272

merged 22 commits into from
Oct 10, 2023

Conversation

rok
Copy link
Contributor

@rok rok commented Sep 13, 2023

See #1199.

Copy link
Contributor

@changhiskhan changhiskhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. is it possible to have a super type interface? e.g., for higher level applications, they may only care about showing an image, or getting a numpy array, and not necessarily care whether the underlying image is represented as string uri, tensor, or bytes.
  2. can we add some methods to convert from one extension type to another?

(Totally fine if the answer is we'll address it later, let's focus on delivering the immediate use case ofc).

@jrabary
Copy link

jrabary commented Sep 14, 2023

Having the ability to decode 16 bits images would be great (for example depth map saved in a png file)

@rok rok force-pushed the 1199 branch 5 times, most recently from 220b2eb to 353e4ef Compare September 19, 2023 13:53
@rok rok marked this pull request as ready for review September 19, 2023 14:13
Copy link
Contributor

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we get docstrings for each of these methods? Preferably with doctest-verified examples.

python/python/lance/arrow.py Outdated Show resolved Hide resolved
python/python/lance/arrow.py Outdated Show resolved Hide resolved
python/python/lance/arrow.py Outdated Show resolved Hide resolved
python/python/lance/arrow.py Outdated Show resolved Hide resolved
python/python/lance/arrow.py Outdated Show resolved Hide resolved
python/python/lance/arrow.py Outdated Show resolved Hide resolved
@wjones127 wjones127 changed the title feat(rust, python): design an image extension type feat(python): design an image extension type Sep 19, 2023
@rok
Copy link
Contributor Author

rok commented Sep 19, 2023

@wjones127 I assume we want to stay in arrow as much as possible, but encoder/encoder and TF bridge use numpy here.
Do you have an idea how to avoid numpy at the arrow <-> numpy bridge?

@wjones127
Copy link
Contributor

I assume we want to stay in arrow as much as possible, but encoder/encoder and TF bridge use numpy here.
Do you have an idea how to avoid numpy at the arrow <-> numpy bridge?

I think there are two concerns:

  1. It would be nice to not require numpy as a dependency.
  2. We should make sure if we aren't having to make data copies unnecessarily.

If this is all internal, I think it's fine to accept this cost for now. We might just have to wait for the ecosystem to get more Arrow native before we can drop numpy as dependency for this.

@rok rok force-pushed the 1199 branch 4 times, most recently from 709698c to 99d73d1 Compare September 20, 2023 04:25
@rok
Copy link
Contributor Author

rok commented Sep 20, 2023

If this is all internal, I think it's fine to accept this cost for now. We might just have to wait for the ecosystem to get more Arrow native before we can drop numpy as dependency for this.

Indeed. Let's keep an eye out for more arrow native ways to move images to tensors while we wait.

Could we get docstrings for each of these methods? Preferably with doctest-verified examples.

Added.

@rok rok requested a review from wjones127 September 20, 2023 04:36
@rok
Copy link
Contributor Author

rok commented Sep 20, 2023

  1. is it possible to have a super type interface? e.g., for higher level applications, they may only care about showing an image, or getting a numpy array, and not necessarily care whether the underlying image is represented as string uri, tensor, or bytes.

I think we should add this now. I'd only propose we do it as a separate PR to manage scope here.

  1. can we add some methods to convert from one extension type to another?

We now have from_uris, read_uris, image_to_tensor, to_tf and to_encoded. Are we missing something we really need at this point? I suppose we are missing pytorch but I'm not sure it's being requested.

@rok
Copy link
Contributor Author

rok commented Sep 20, 2023

@changhiskhan

  1. is it possible to have a super type interface? e.g., for higher level applications, they may only care about showing an image, or getting a numpy array, and not necessarily care whether the underlying image is represented as string uri, tensor, or bytes.

I think we should add this now. I'd only propose we do it as a separate PR to manage scope here.

  1. can we add some methods to convert from one extension type to another?

We now have from_uris, read_uris, image_to_tensor, to_tf and to_encoded. Are we missing something we really need at this point? I suppose we are missing pytorch but I'm not sure it's being requested.

@rok rok force-pushed the 1199 branch 3 times, most recently from 5ef6130 to 5cf5a23 Compare September 20, 2023 15:18
Copy link
Contributor

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most important change is that we add a roundtrip test to Lance.

python/python/lance/arrow.py Outdated Show resolved Hide resolved
python/python/lance/arrow.py Show resolved Hide resolved
python/python/lance/arrow.py Outdated Show resolved Hide resolved
python/python/lance/arrow.py Outdated Show resolved Hide resolved
python/python/lance/arrow.py Outdated Show resolved Hide resolved
python/python/lance/arrow.py Outdated Show resolved Hide resolved
python/python/lance/arrow.py Show resolved Hide resolved
Copy link
Contributor

@westonpace westonpace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool, only a few thoughts

python/python/lance/arrow.py Outdated Show resolved Hide resolved
python/python/lance/arrow.py Show resolved Hide resolved
python/python/lance/arrow.py Outdated Show resolved Hide resolved

def image_to_tensor(self, decoder=None):
"""
Decode encoded images and return a FixedShapeImageTensorArray
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm...if I'm a user I might wonder what the shape of the resulting array would be. It looks like, for both tensorflow and pillow, you get a 3d array [height, width, channel]. Do you think we should document and/or mandate this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The annoying part here is that PNG comes with an extra transparency channel etc. Since we're offering a custom decoder option it'd be awkward to enforce output shapes here.
I'm not sure what a good user experience here. Maybe we could get some user feedback?

@rok
Copy link
Contributor Author

rok commented Sep 25, 2023

FixedShapeTensorType can't be subclassed at the moment, so we need to consider other options for FixedShapeImageTensor. Here are some ideas:

  • Use vanilla FixedShapeTensorType and hence FixedShapeTensorArray. Downside is that we can't add methods to these, but we could instead provide a utility module for images. This makes image types (uri, encoded, tensor) less uniform, but we're probably adding a universal image class on top of them anyway.
  • Wait for arrow to add a way to subclass canonical extensions to add ImageTensor type.
  • Create our own extension class that is a copy of FixedShapeTensorType. This would work immediately and make API nicer. We would miss some methods FixedShapeTensorArray implements in C++, but we could still use those with on the fly casting.

In related news VariableShapeTensorType proposal will go to vote soon.

python/python/lance/arrow.py Outdated Show resolved Hide resolved
@rok rok requested a review from wjones127 October 3, 2023 22:18
@rok
Copy link
Contributor Author

rok commented Oct 9, 2023

@wjones127 can we merge this?

@wjones127 wjones127 merged commit 8f78332 into lancedb:main Oct 10, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants