Skip to content

refactor: comp backend #1030

Merged
samsja merged 7 commits intofeat-rewrite-v2from
refactor-comp-backend
Jan 18, 2023
Merged

refactor: comp backend #1030
samsja merged 7 commits intofeat-rewrite-v2from
refactor-comp-backend

Conversation

@samsja
Copy link
Copy Markdown
Member

@samsja samsja commented Jan 17, 2023

Context

Because of some type problem we need multiples times in the codebase to duplicate function definition in the Computional Backend part.

This PR fix this by doing a refactoring on the type part. The new one is less precise and correct that before but more flexible and easier to maintain.

Main difference are two folds:

  • Use Instance instead of class in get_comp_backend (to fix the static method pb)
  • The return type of the function in the computational backend is always np.ndarray or torch.Tensor. Therefore mypy cannot now that if you passed a TorchTensor (from us) the method will return a TorchTensor. This is a limitation but it allow us to remove the second TypeVar Generic in the Computational backend. This means in some case we need either to tell mypy we know the return type will be a TorchTensor by calling cast or TorchTensor._docarray_from_native()

Signed-off-by: Sami Jaghouar <[email protected]>
Signed-off-by: Sami Jaghouar <[email protected]>
Signed-off-by: Sami Jaghouar <[email protected]>
@samsja samsja changed the title Refactor comp backend refactor: comp backend Jan 17, 2023
@Jackmin801
Copy link
Copy Markdown
Contributor

Other than that LGTM

@anna-charlotte
Copy link
Copy Markdown
Contributor

Nice, I like it, I think we can remove most of the # type: ignore's now, too, e.g. in docarray/typing/tensor/video/video_tensor_mixin.py

from docarray.computation.numpy_backend import NumpyCompBackend

return NumpyCompBackend
return NumpyCompBackend()
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.

Is it a bit of an anti pattern to crate all of these instances if we don't really need them? would it make sense to maintain a singleton instance?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh that is a good idea actually. I support this. Lets go singleton with this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

okay good idea

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

actually doing the singleton is not that trivial in python ... I think it adds complexity for few real advantages at the end. Only conceptual advantage.

@github-actions
Copy link
Copy Markdown

📝 Docs are deployed on https://ft-refactor-comp-backend--jina-docs.netlify.app 🎉

@samsja samsja merged commit 13d0aec into feat-rewrite-v2 Jan 18, 2023
@samsja samsja deleted the refactor-comp-backend branch January 18, 2023 13:33
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.

4 participants