GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — fix: raise exception when type of DocList is object by punndcoder28 · Pull Request #1794 · docarray/docarray

This repository was archived by the owner on Sep 9, 2026. It is now read-only.

Conversation

punndcoder28 commented Sep 16, 2023

Copy link

Copy Markdown

Contributor

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'>

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

punndcoder28 commented

Sep 16, 2023

View reviewed changes

Comment thread

docarray/array/doc_list/doc_list.py Outdated

		else:
		return super().__class_getitem__(item)

		if isinstance(item, object) and not isinstance(item, TypeVar):

punndcoder28 Sep 16, 2023

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

punndcoder28 marked this pull request as ready for review

September 16, 2023 18:01

punndcoder28 commented Sep 16, 2023

Copy link

Copy Markdown

Contributor Author

@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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

codecov Bot commented Sep 16, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JoanFM commented Sep 17, 2023

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

punndcoder28 force-pushed the fix_bug_doclist_type branch from 08b7e7b to 8621817 Compare

September 18, 2023 12:44

JoanFM linked an issue

Sep 18, 2023

that may be closed by this pull request

Bug with DocList[] #1782

Closed

JoanFM suggested changes

Sep 18, 2023

View reviewed changes

Comment thread

docarray/array/doc_list/doc_list.py

		return AnyDocArray.__class_getitem__.__func__(cls, item) # type: ignore
		else:
		return super().__class_getitem__(item)
		if (

JoanFM Sep 18, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

punndcoder28 Sep 18, 2023

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

punndcoder28 commented Sep 18, 2023

Copy link

Copy Markdown

Contributor Author

Will fix the failing tests related to new changes

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

punndcoder28 commented Sep 18, 2023

Copy link

Copy Markdown

Contributor Author

@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']:

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

punndcoder28 force-pushed the fix_bug_doclist_type branch from 2144b3c to 1a727be Compare

September 18, 2023 17:02

JoanFM commented Sep 18, 2023

Copy link

Copy Markdown

Member

@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

samsja reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          fix: raise exception when type of DocList is object

Signed-off-by: punndcoder28 <[email protected]>

punndcoder28 force-pushed the fix_bug_doclist_type branch from 1a727be to 8266238 Compare

September 19, 2023 16:36


          Merge branch 'main' into fix_bug_doclist_type

2c41809

punndcoder28 requested a review from JoanFM

September 19, 2023 17:03

JoanFM approved these changes

Sep 19, 2023

View reviewed changes

JoanFM merged commit 2f3b85e into docarray:main

Sep 19, 2023

JoanFM mentioned this pull request

Oct 2, 2023

Release Notes #1817

Closed

Reviewers

1 more reviewer

JoanFM

JoanFM approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

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[]

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

punndcoder28 commented Sep 16, 2023

Uh oh!

punndcoder28 Sep 16, 2023

Choose a reason for hiding this comment

Uh oh!

punndcoder28 commented Sep 16, 2023

Uh oh!

codecov Bot commented Sep 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JoanFM commented Sep 17, 2023

Uh oh!

JoanFM Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

punndcoder28 Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

punndcoder28 commented Sep 18, 2023

Uh oh!

punndcoder28 commented Sep 18, 2023

Uh oh!

JoanFM commented Sep 18, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Sep 16, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.