Skip to content

fix: better error message when docvec is unusable #1675

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

Merged
merged 3 commits into from
Jun 27, 2023

Conversation

JohannesMessner
Copy link
Member

closes #1563

The approach is to set a flag _is_unasable as soon as the instance gets unusable, and from that point onward raise an Exception at every getatts/setattr interaction.

Unfortunately we cannot override getattr/setattr on the fly (and get rid of the _is_unasable) flag: https://stackoverflow.com/questions/10376604/overriding-special-methods-on-an-instance

Signed-off-by: Johannes Messner <[email protected]>
@JohannesMessner JohannesMessner requested review from samsja and JoanFM June 27, 2023 15:30
@JohannesMessner JohannesMessner marked this pull request as ready for review June 27, 2023 15:31
@@ -32,6 +33,12 @@
T_doc = TypeVar('T_doc', bound=BaseDoc)
IndexIterType = Union[slice, Iterable[int], Iterable[bool], None]

UNUSABLE_ERROR_MSG = (
'This {cls} instance is in an unusable state. \n'
'The most common cause of this is converting a DocVec to a DocList. '
Copy link
Member Author

Choose a reason for hiding this comment

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

Right now this is the only cause, actually, but I wanted to keep our options open here.

Signed-off-by: Johannes Messner <[email protected]>
Copy link
Member

@JoanFM JoanFM left a comment

Choose a reason for hiding this comment

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

Why is it unusable?

@github-actions
Copy link

📝 Docs are deployed on https://ft-fix-vec-to-list-error-msg--jina-docs.netlify.app 🎉

@JohannesMessner
Copy link
Member Author

Why is it unusable?

Because to_doc_list() directly modifies the data stored in the columns in order to not copy the data. This "destroys" the columns.
@samsja knows more about the implementation details.

@JoanFM JoanFM merged commit bcb60ca into main Jun 27, 2023
@JoanFM JoanFM deleted the fix-vec-to-list-error-msg branch June 27, 2023 16:09
@JoanFM JoanFM mentioned this pull request Jul 3, 2023
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.

bug: better error message after calling to_doc_list on DocVec
2 participants