Skip to content

chore(document): change type hint for tags in docarray.document.data.…#735

Merged
JoanFM merged 2 commits intodocarray:mainfrom
marcosbodio:chore-document-732
Nov 10, 2022
Merged

chore(document): change type hint for tags in docarray.document.data.…#735
JoanFM merged 2 commits intodocarray:mainfrom
marcosbodio:chore-document-732

Conversation

@marcosbodio
Copy link
Copy Markdown
Contributor

@marcosbodio marcosbodio commented Nov 8, 2022

…DocumentData

Goals:

self._data.tags = value

@property
def _metadata(self) -> Optional[Dict[str, 'StructValueType']]:
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.

why are you changing this here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

_property.py is auto-generated using scripts/gen_doc_property_mixin.py. This line is the same as the code in the main branch https://github.com/docarray/docarray/blob/main/docarray/document/mixins/_property.py.

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.

okey,

Copy link
Copy Markdown
Contributor Author

@marcosbodio marcosbodio Nov 8, 2022

Choose a reason for hiding this comment

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

@JoanFM sorry, my mistake. I double-checked, and getter and setter for _metadata are missing in the version of _property.py in my branch because the script scripts/gen_doc_property_mixin.py has the following code

    for f in fields(DocumentData):
        if f.name.startswith('_'):
            continue
   ....

(see https://github.com/docarray/docarray/blob/main/scripts/gen_doc_property_mixin.py)

and therefore it skips all fields starting with _. I do not know why you have those getter and setter in the main branch. I can remove the if in the script, but in that case the script would generate also getter and setter for other fields of Document starting with _, such as _reference_doc. Alternatively, I may add the getter and setter for _metadata manually.

What do you prefer? Any other idea?

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.

Let's add them manually

from docarray import DocumentArray
from docarray.typing import ArrayType, StructValueType, DocumentContentType
if TYPE_CHECKING:
from ...score import NamedScore
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.

please keep the absolute imports

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can do that: it is a simple change to the script scripts/gen_doc_property_mixin.py

marcosbodio added a commit to marcosbodio/docarray that referenced this pull request Nov 9, 2022
@marcosbodio marcosbodio requested a review from JoanFM November 9, 2022 17:39
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Nov 10, 2022

Codecov Report

Base: 88.29% // Head: 79.80% // Decreases project coverage by -8.49% ⚠️

Coverage data is based on head (8515311) compared to base (f4b16ef).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #735      +/-   ##
==========================================
- Coverage   88.29%   79.80%   -8.50%     
==========================================
  Files         134      133       -1     
  Lines        6648     6655       +7     
==========================================
- Hits         5870     5311     -559     
- Misses        778     1344     +566     
Flag Coverage Δ
docarray 79.80% <100.00%> (-8.50%) ⬇️

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

Impacted Files Coverage Δ
docarray/array/qdrant.py 100.00% <ø> (ø)
docarray/document/data.py 94.68% <100.00%> (ø)
docarray/document/mixins/_property.py 85.80% <100.00%> (-2.79%) ⬇️
docarray/array/mixins/embed.py 9.89% <0.00%> (-82.42%) ⬇️
docarray/array/mixins/evaluation.py 9.09% <0.00%> (-79.03%) ⬇️
docarray/array/mixins/io/pushpull.py 31.89% <0.00%> (-61.21%) ⬇️
docarray/array/mixins/io/dataframe.py 41.66% <0.00%> (-58.34%) ⬇️
docarray/array/storage/memory/find.py 42.59% <0.00%> (-51.86%) ⬇️
docarray/array/mixins/text.py 50.00% <0.00%> (-50.00%) ⬇️
docarray/array/mixins/io/common.py 26.31% <0.00%> (-42.11%) ⬇️
... and 39 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JoanFM JoanFM linked an issue Nov 10, 2022 that may be closed by this pull request
@JoanFM JoanFM merged commit a2ce9bb into docarray:main Nov 10, 2022
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 this pull request may close these issues.

chore: change type hint for tags in docarray.document.data.DocumentData

3 participants