Skip to content

Comments

fix: raise exception when type of DocList is object#1794

Merged
JoanFM merged 2 commits intodocarray:mainfrom
punndcoder28:fix_bug_doclist_type
Sep 19, 2023
Merged

fix: raise exception when type of DocList is object#1794
JoanFM merged 2 commits intodocarray:mainfrom
punndcoder28:fix_bug_doclist_type

Conversation

@punndcoder28
Copy link
Contributor

@punndcoder28 punndcoder28 commented Sep 16, 2023

  • Fixes being able to pass an object for DocList without causing code to raise expection as mentioned in Bug with DocList[] #1782

Test Plan - Running the below code raises exception

from docarray import DocList, BaseDoc

doc = DocList[BaseDoc()]

Exception raise

Traceback (most recent call last):
  File "test.py", line 3, in <module>
    doc = DocList[BaseDoc()]
  File "docarray/docarray/array/doc_list/doc_list.py", line 340, in __class_getitem__
    raise TypeError(f'Expecting a type, got {item} of type {type(item)}')
TypeError: Expecting a type, got BaseDoc(id='9ab01e333a55195e5bb3f97604b97f10') of type <class 'docarray.base_doc.doc.BaseDoc'>

punndcoder28
punndcoder28 commented Sep 16, 2023
View reviewed changes
docarray/array/doc_list/doc_list.py Outdated
else:
return super().__class_getitem__(item)

if isinstance(item, object) and not isinstance(item, TypeVar):
Copy link
Contributor Author

@punndcoder28 punndcoder28 Sep 16, 2023

Choose a reason for hiding this comment

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

added a check isinstance(item, TypeVar) since item of TypeVar was also being passed when __class_getitem__ was called

@punndcoder28 punndcoder28 marked this pull request as ready for review September 16, 2023 18:01
@punndcoder28
Copy link
Contributor Author

punndcoder28 commented Sep 16, 2023

@samsja @JoanFM I am unsure on how to handle the case doc = DocList[MyDoc][MyDoc] because the item parameter which gets passed to __class_getitem__ of both AnyDocArray and DocList is of type BaseDoc

@codecov
Copy link

codecov bot commented Sep 16, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.56% 🎉

Comparison is base (8f32866) 84.24% compared to head (2c41809) 84.81%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1794      +/-   ##
==========================================
+ Coverage   84.24%   84.81%   +0.56%     
==========================================
  Files         135      135              
  Lines        8899     8901       +2     
==========================================
+ Hits         7497     7549      +52     
+ Misses       1402     1352      -50
Flag Coverage Δ
docarray 84.81% <100.00%> (+0.56%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
docarray/array/doc_list/doc_list.py 86.99% <100.00%> (+0.21%) ⬆️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JoanFM
Copy link
Member

JoanFM commented Sep 17, 2023

Hey @punndcoder28, please make sure to add relevant tests for this

@JoanFM JoanFM linked an issue Sep 18, 2023 that may be closed by this pull request
Bug with DocList[] #1782
Closed
JoanFM
JoanFM requested changes Sep 18, 2023
View reviewed changes
docarray/array/doc_list/doc_list.py
return AnyDocArray.__class_getitem__.__func__(cls, item) # type: ignore
else:
return super().__class_getitem__(item)
if (
Copy link
Member

@JoanFM JoanFM Sep 18, 2023

Choose a reason for hiding this comment

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

should we better have an elif?

Copy link
Contributor Author

@punndcoder28 punndcoder28 Sep 18, 2023

Choose a reason for hiding this comment

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

Since each condition has a terminating condition from the function wouldn't multiple if's be similar to elif? If there was a scenario like below an elif would definitely make it much better. In the above case, we check the instance of item and based on the condition we return a type or raise an exception. But if the code base follows if ... elif ... else blocks I would love to keep it uniform so that it is easy to maintain

computation
if condn1:
   computation = value1
elif condn2:
   computation = value2
else:
   computation = value3

return computation

@punndcoder28
Copy link
Contributor Author

punndcoder28 commented Sep 18, 2023

Will fix the failing tests related to new changes

@punndcoder28
Copy link
Contributor Author

punndcoder28 commented Sep 18, 2023

@JoanFM Can we pass string literal as types for a class? Like so

    def g(a: DocList['BaseDoc']) -> DocList['BaseDoc']:
        return a

This is being done here

docarray/tests/units/array/test_array.py

Line 383 in 8f32866

def g(a: DocList['BaseDoc']) -> DocList['BaseDoc']:

@JoanFM
Copy link
Member

JoanFM commented Sep 18, 2023

@JoanFM Can we pass string literal as types for a class? Like so

    def g(a: DocList['BaseDoc']) -> DocList['BaseDoc']:
        return a

This is being done here

docarray/tests/units/array/test_array.py

Line 383 in 8f32866

def g(a: DocList['BaseDoc']) -> DocList['BaseDoc']:

u can in the type hints

@punndcoder28
fix: raise exception when type of DocList is object
8266238 
Signed-off-by: punndcoder28 <puneethk.2899@gmail.com>
@punndcoder28 punndcoder28 requested a review from JoanFM September 19, 2023 17:03
@JoanFM JoanFM merged commit 2f3b85e into docarray:main Sep 19, 2023
@JoanFM JoanFM mentioned this pull request Oct 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JoanFM JoanFM JoanFM approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug with DocList[]

2 participants

@punndcoder28 @JoanFM