Skip to content

feat: push meta data along with docarray 0.16#490

Merged
alaeddine-13 merged 24 commits intomainfrom
feat-push-metadata
Sep 16, 2022
Merged

feat: push meta data along with docarray 0.16#490
alaeddine-13 merged 24 commits intomainfrom
feat-push-metadata

Conversation

@hanxiao
Copy link
Copy Markdown
Member

@hanxiao hanxiao commented Aug 17, 2022

Note: Please hold your review on this PR, there is no point in thinking too much about this PR until we get the Hub UI out, otherwise it is overengineering. In other words, we need to get first version of Hub UI this PR merged, before make meaningful progress.

leverage hubble metaData field and upload meta informatio, so that the Console Data GUI is able to show preview info directly.

metaData is contains branding and summary field,

use print(DocumentArray.empty(10).push('your-name-12345')) to see the full payload

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 17, 2022

Codecov Report

Merging #490 (88406a1) into main (ea2a7a8) will increase coverage by 1.93%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##             main     #490      +/-   ##
==========================================
+ Coverage   84.47%   86.40%   +1.93%     
==========================================
  Files         142      142              
  Lines        7087     7137      +50     
==========================================
+ Hits         5987     6167     +180     
+ Misses       1100      970     -130     
Flag Coverage Δ
docarray 86.40% <95.00%> (+1.93%) ⬆️

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

Impacted Files Coverage Δ
docarray/helper.py 82.66% <89.28%> (+13.88%) ⬆️
docarray/array/mixins/io/pushpull.py 92.17% <97.91%> (+4.01%) ⬆️
docarray/array/mixins/plot.py 67.09% <100.00%> (-0.92%) ⬇️
docarray/proto/pb2/docarray_pb2.py 60.37% <0.00%> (-39.63%) ⬇️
docarray/array/mixins/traverse.py 95.04% <0.00%> (+4.13%) ⬆️
docarray/document/mixins/sugar.py 68.00% <0.00%> (+12.00%) ⬆️
docarray/array/mixins/group.py 87.17% <0.00%> (+17.94%) ⬆️
docarray/array/mixins/post.py 93.02% <0.00%> (+74.41%) ⬆️
... and 1 more

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

@hanxiao hanxiao changed the title feat: push meta data along with docarray feat: push meta data along with docarray 0.16 Aug 18, 2022
@hanxiao hanxiao added the 0.16 label Aug 18, 2022
da = da_cls.empty(100)
da._get_attributes()
da.summary()
da._get_raw_summary()
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.

unlike summary, I think here the output can be tested

Copy link
Copy Markdown
Member

@JohannesMessner JohannesMessner left a comment

Choose a reason for hiding this comment

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

Some documentation would be nice!


@classmethod
@hubble.login_required
def cloud_list(cls, show_table: bool = False) -> List[str]:
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.

Am I understanding correctly that the intended usage is to do DocumentArray.cloud_list() (same for cloud_delete())?
I am not sure if it is super intuitive to the user to interact with the DocumentArray class when what they are doing is communicating with a server. In my head this operation has not much to do with DocumentArray.

Could this be just a function that the user imports directly?

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.

I think static method looks more comprehensive

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.

I think it is better to be a staticmethod and not a classmethod. You do not use cls for anything right?

@hanxiao
Copy link
Copy Markdown
Member Author

hanxiao commented Aug 20, 2022

Please hold your review on this PR, there is no point in thinking too much about this PR until we get the Hub UI out. All other work is overengineering. In other words, we need to get first version of Hub UI this PR merged, before make meaningful progress.

@JoanFM JoanFM marked this pull request as draft September 8, 2022 07:52
@alaeddine-13 alaeddine-13 force-pushed the feat-push-metadata branch 2 times, most recently from 18e130c to 7b008c1 Compare September 14, 2022 15:33
@alaeddine-13
Copy link
Copy Markdown
Member

Some documentation would be nice!

documentation added

@alaeddine-13 alaeddine-13 marked this pull request as ready for review September 15, 2022 07:28

@classmethod
@hubble.login_required
def cloud_list(cls, show_table: bool = False) -> List[str]:
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.

I think it is better to be a staticmethod and not a classmethod. You do not use cls for anything right?

Copy link
Copy Markdown
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.

LGTM, just minor thing

@github-actions
Copy link
Copy Markdown

📝 Docs are deployed on https://ft-feat-push-metadata--jina-docs.netlify.app 🎉

@alaeddine-13 alaeddine-13 merged commit 14526db into main Sep 16, 2022
@alaeddine-13 alaeddine-13 deleted the feat-push-metadata branch September 16, 2022 10:07
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.

4 participants