FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat(v2): add tensorflow support by anna-charlotte · Pull Request #1064 · docarray/docarray · GitHub

/ docarray Public

feat(v2): add tensorflow support - #1064

Merged
samsja merged 74 commits into
feat-rewrite-v2from
feat-tensorflow-support
Feb 8, 2023
ConversationCommits (74)ChecksFiles changed
Merged

feat(v2): add tensorflow support#1064
samsja merged 74 commits into
feat-rewrite-v2from
feat-tensorflow-support

Conversation

anna-charlotte commented Jan 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Signed-off-by: anna-charlotte charlotte.gerhaher@jina.ai

Goals:

Add Tensorflow support to v2:

  • Add TF tensor type
    • As of right now, I haven't found a solution on how to subclass a tf.Tensor (see here), so for now we weill store the tf.Tensor instance as an attribute of a TensorFlowTensor instance.
  • Add TF computational backend
  • check and update documentation, if required. See guide

anna-charlotte linked an issue Jan 30, 2023 that may be closed by this pull request
v2: Add Tensforflow support #1057
Closed
3 tasks
JohannesMessner reviewed Jan 30, 2023
View reviewed changes
Comment thread docarray/typing/tensor/tensorflow_tensor.py Outdated
Comment thread docarray/typing/tensor/tensorflow_tensor.py Outdated

github-actions Bot commented Jan 30, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

2 similar comments

github-actions Bot commented Jan 31, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions Bot commented Jan 31, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions Bot added size/l and removed size/xl labels Jan 31, 2023
anna-charlotte mentioned this pull request Feb 1, 2023
Closed
47 tasks
github-actions Bot added size/xl and removed size/l labels Feb 1, 2023

github-actions Bot commented Feb 1, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

anna-charlotte force-pushed the feat-tensorflow-support branch from e410061 to efac716 Compare February 1, 2023 18:34

github-actions Bot commented Feb 1, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

3 similar comments

github-actions Bot commented Feb 2, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions Bot commented Feb 2, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions Bot commented Feb 2, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

samsja reviewed Feb 2, 2023
View reviewed changes
Comment thread docarray/computation/tensorflow_backend.py
return t.tensor


class TensorFlowCompBackend(AbstractNumpyBasedBackend[TensorFlowTensor]):

samsja Feb 2, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

general comment about the class, to me it looks good I like that we rely on tnp as much as we can.

Maybe norm_left, norm_right should be named differenlty

norm_left could be : cast_output
norm_right could be: get_tensor

this will make the code clearer

maybe @JohannesMessner as a better idea for the name.

samsja reviewed Feb 2, 2023
View reviewed changes

samsja left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I added few comments

anna-charlotte force-pushed the feat-tensorflow-support branch from 5352804 to 494a9eb Compare February 3, 2023 16:05

github-actions Bot commented Feb 3, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions Bot commented Feb 8, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

docs: update readme.md
c2aa0b1 
Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>

github-actions Bot commented Feb 8, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

fix: remove n dim from abstract method instead use comp be
81a2540 
Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>

github-actions Bot commented Feb 8, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

fix: remove proto mark, because only test for proto 3 here
838955a 
Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>

github-actions Bot commented Feb 8, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

fix: tf set item and add tests
09dd6a9 
Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>

github-actions Bot commented Feb 8, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

JohannesMessner requested changes Feb 8, 2023
View reviewed changes
Comment thread docarray/array/array_stacked.py
Comment on lines +36 to +39
try:
import tensorflow as tf # type: ignore

from docarray.typing import TensorFlowTensor

JohannesMessner Feb 8, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

for torch i moved this thing to a helper in utils, so this check only has to be done once globally. Can we do the same for tf?

anna-charlotte Feb 8, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yes, saw that and started doing this in the TF embedding/video/audio PR, so i'll do this refactor there if that's fine with you @JohannesMessner

github-actions Bot commented Feb 8, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

anna-charlotte marked this pull request as ready for review February 8, 2023 11:19
JoanFM reviewed Feb 8, 2023
View reviewed changes
Comment thread README.md Outdated
return inputs
```

Much nicer, don't you think? One main difference to using DocArray with PyTorch, is that when using TensorFlowTensor's, you have to access it's `.tensor` attribute directly, as it can be seen in `.forward_podcast()` above. This is due to the fact that while `TorchTensor` is a subclass of `torch.Tensor`, `TensorFlowTensor` is not a subclass of `tf.Tensor` but instead stores a `tf.Tensor` in its `.tensor` attribute.

JoanFM Feb 8, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It may be good to explain why this is the case?

JoanFM Feb 8, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why is this different?

samsja Feb 8, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why is this different?
@anna-charlotte you can probably add due to a technical limitation on tf.Tensor

github-actions Bot commented Feb 8, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

samsja reviewed Feb 8, 2023
View reviewed changes
Comment thread README.md
schema definition (see below). Everything handles in a pythonic manner by relying on type hints.


## Coming from TensorFlow

samsja Feb 8, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this part is too big IMO. We just need to show that there is a (tiny ?) difference and that you need to access tensor.tensor. No need to show the full example

Comment thread README.md Outdated
return inputs
```

Much nicer, don't you think? One main difference to using DocArray with PyTorch, is that when using TensorFlowTensor's, you have to access it's `.tensor` attribute directly, as it can be seen in `.forward_podcast()` above. This is due to the fact that while `TorchTensor` is a subclass of `torch.Tensor`, `TensorFlowTensor` is not a subclass of `tf.Tensor` but instead stores a `tf.Tensor` in its `.tensor` attribute.

samsja Feb 8, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why is this different?
@anna-charlotte you can probably add due to a technical limitation on tf.Tensor

docs: update tf section in readme.md
85183ec 
Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>

github-actions Bot commented Feb 8, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

anna-charlotte reopened this Feb 8, 2023

github-actions Bot commented Feb 8, 2023

Copy link
Copy Markdown

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

github-actions Bot commented Feb 8, 2023

Copy link
Copy Markdown

📝 Docs are deployed on https://ft-feat-tensorflow-support--jina-docs.netlify.app 🎉

1 similar comment

github-actions Bot commented Feb 8, 2023

Copy link
Copy Markdown

📝 Docs are deployed on https://ft-feat-tensorflow-support--jina-docs.netlify.app 🎉

JohannesMessner approved these changes Feb 8, 2023
View reviewed changes

JohannesMessner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

samsja merged commit 6b0d13c into feat-rewrite-v2 Feb 8, 2023
samsja deleted the feat-tensorflow-support branch February 8, 2023 14:27
anna-charlotte mentioned this pull request Feb 20, 2023
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

JoanFM JoanFM left review comments
JohannesMessner JohannesMessner approved these changes
samsja samsja approved these changes

Assignees

No one assigned

Labels

area/cicd area/core area/housekeeping area/setup area/testing area/typing component/array size/xl

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

v2: Add Tensforflow support

4 participants

Back | FazBrowse Home | New Git URL