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

DocList raises exception for type object. #1828

Open
3 of 6 tasks
corentinmarek opened this issue Oct 30, 2023 · 3 comments · May be fixed by #1829
Open
3 of 6 tasks

DocList raises exception for type object. #1828

corentinmarek opened this issue Oct 30, 2023 · 3 comments · May be fixed by #1829

Comments

@corentinmarek
Copy link

corentinmarek commented Oct 30, 2023

Initial Checks

  • I have read and followed the docs and still think this is a bug

Description

This commit introduced a check to verify that DocList is not used with an object:

        if (
            isinstance(item, object)
            and not is_typevar(item)
            and not isinstance(item, str)
            and item is not Any
        ):
               raise TypeError('Expecting a type, got object instead')

This is quite a broad condition (it breaks things like DocList[TorchTensor], or nested DocList[DocList[...]] for me for instance) as:

  • Almost everything will be an object so the first line if almost a catch-all.
  • is_typevar only checks for TypeVar objects.

Should this not check as well for something not isinstance(item, type) instead of not is_typevar to allow for classes ?
This way only non class objects (like instances of class object that are not classes themselves) will raise the TypeError.

Example Code

from docarray import DocList
from docarray.typing import TorchTensor
test = DocList[TorchTensor]

Python, DocArray & OS Version

0.39.1

Affected Components

@JoanFM
Copy link
Member

JoanFM commented Oct 30, 2023

Hello @corentinmarek,

It would be very helpful if you could provide a draft PR with your idea for a better implementatiom of this.

@corentinmarek
Copy link
Author

👍: #1829. Waiting for checks.

@JoanFM JoanFM linked a pull request Oct 31, 2023 that will close this issue
@JoanFM
Copy link
Member

JoanFM commented Oct 31, 2023

However, I am not sure DocList[TorchTensor] and DocList[DocList[...]] will work as now, because they are not BaseDoc

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 a pull request may close this issue.

2 participants