Skip to content

docs: add torch and tf tensors to Api references section#1345

Merged
anna-charlotte merged 15 commits into
feat-rewrite-v2from
docs-add-torch-and-tf-tensors
Apr 11, 2023
Merged

docs: add torch and tf tensors to Api references section#1345
anna-charlotte merged 15 commits into
feat-rewrite-v2from
docs-add-torch-and-tf-tensors

Conversation

@anna-charlotte

@anna-charlotte anna-charlotte commented Apr 6, 2023

Copy link
Copy Markdown
Contributor

Goals:

Currently all tf and torch tensors do not appear in the Api references section. I think this might be due to the fact that those are not included in the __init__.py's __all__ list.

For instance in the docarray.typing.tensor.__init__.py the tf and torch tensors will be added only via the __get_attr__(). Mkdocstring does not seem to find those.

So I think they have to be added explicitly.
My current solution:
Split tensor section into AudioTensor, ImageTensor, Embedding, VideoTensor and Tensor. For each section I'll add the files explicitly:

# AudioTensor

::: docarray.typing.tensor.audio.audio_ndarray
::: docarray.typing.tensor.audio.abstract_audio_tensor
::: docarray.typing.tensor.audio.audio_tensorflow_tensor
::: docarray.typing.tensor.audio.audio_torch_tensor

This however results in a not so great looking table of contents (on the right):
image

For each file, the filepath is listed, as well as the class inside of it.

Update:
I just realized that that is the same for some of the other sections, so I guess it is fine like this

  • check and update documentation, if required. See guide

Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>
@anna-charlotte anna-charlotte marked this pull request as ready for review April 6, 2023 16:02
Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>
anna-charlotte added 4 commits April 6, 2023 19:11
Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>
Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>
Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>
Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>
@github-actions github-actions Bot added size/l and removed size/m labels Apr 11, 2023
Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>
@anna-charlotte anna-charlotte mentioned this pull request Apr 11, 2023
4 tasks
anna-charlotte added 3 commits April 11, 2023 09:59
Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>
Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>
Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>

@samsja 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.

lgtm, approving so that you can merge when test pass

Comment thread docarray/typing/__init__.py Outdated
'AudioTorchTensor',
'VideoTorchTensor',
]
__all__.extend(_torch_tensors)

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.

@samsja The torch tensors were not included in the docstring tests, because they were not found in __alll__. Is it okay to add them to all outside of the following __getattr__? In this case, they are included in all also if torch is not installed.

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.

hmmm not okay I would say

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.

best way would be to create a __all_test__ for them or just to write a custom docstring test just for the tensor thing

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.

in the test, you could do smth like this.

`all_test == hasattr(module, 'all_test') or hasattr(module, 'all')

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.

how about this:

if "pytest" in sys.modules:
    __all__.extend(_torch_tensors)

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.

hmmm not sure. This means

from docarray.typing import *

TorchTensor()

has a different behavior inside and outside pytest. It kind of go against the principle of tests. Something could go wrong outside of pytest but not in pytest

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.

ok, I will go with the __all_test__ option that you suggested!

Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>
anna-charlotte added 2 commits April 11, 2023 11:48
Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>
Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>
anna-charlotte added 2 commits April 11, 2023 12:12
Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>
Signed-off-by: anna-charlotte <charlotte.gerhaher@jina.ai>
@github-actions

Copy link
Copy Markdown

📝 Docs are deployed on https://ft-docs-add-torch-and-tf-tensors--jina-docs.netlify.app 🎉

@anna-charlotte anna-charlotte merged commit 0e18796 into feat-rewrite-v2 Apr 11, 2023
@anna-charlotte anna-charlotte deleted the docs-add-torch-and-tf-tensors branch April 11, 2023 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants