Skip to content
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

refactor(hubio): push, login, docker auth #1691

Merged
merged 2 commits into from Jan 14, 2021
Merged

refactor(hubio): push, login, docker auth #1691

merged 2 commits into from Jan 14, 2021

Conversation

deepankarm
Copy link
Member

No description provided.

@deepankarm deepankarm requested a review from a team as a code owner January 14, 2021 16:10
@jina-bot jina-bot added size/L area/core This issue/PR affects the core codebase area/helper This issue/PR affects the helper functionality area/testing This issue/PR affects testing component/docker labels Jan 14, 2021
@codecov
Copy link

codecov bot commented Jan 14, 2021

Codecov Report

Merging #1691 (ba93743) into master (5530733) will increase coverage by 0.60%.
The diff coverage is 78.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1691      +/-   ##
==========================================
+ Coverage   84.20%   84.80%   +0.60%     
==========================================
  Files         132      133       +1     
  Lines        6830     6820      -10     
==========================================
+ Hits         5751     5784      +33     
+ Misses       1079     1036      -43     
Impacted Files Coverage Δ
jina/docker/checker.py 84.90% <ø> (+2.76%) ⬆️
jina/docker/hubio.py 70.27% <74.35%> (+7.00%) ⬆️
jina/docker/hubapi/remote.py 75.32% <75.32%> (ø)
jina/parsers/hub/__init__.py 65.00% <80.00%> (-0.12%) ⬇️
jina/docker/hubapi/local.py 81.70% <81.70%> (ø)
jina/parsers/hub/login.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5530733...ba93743. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Jan 14, 2021

Latency summary

Current PR yields:

  • 😶 index QPS at 1645, delta to last 3 avg.: -3%
  • 😶 query QPS at 29, delta to last 3 avg.: +0%

Breakdown

Version Index QPS Query QPS
current 1645 29
0.9.13 1688 29
0.9.12 1706 29

Backed by latency-tracking. Further commits will update this comment.

'try "git submodule update --init" if you are in dev mode')
return {}

def add_hub():
Copy link
Member

Choose a reason for hiding this comment

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

pass hub_images as param, otherwise is impossible to understand, u are refering hub_images before instantiation.

Copy link
Member

Choose a reason for hiding this comment

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

I actually I think is more elegant to have get_hub and have it return a key, value tuple to put in ur hub_images and so u do not pass hub_images as argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is restructuring of the existing code. I would do this in the next PR, while fixing list

return {}

def add_hub():
m_yml = f'{info.module_finder.path}/manifest.yml'
Copy link
Member

Choose a reason for hiding this comment

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

os.path.join I think is better

Copy link
Member Author

Choose a reason for hiding this comment

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

Same.

f'please re-login using command: {colored("jina hub login", attrs=["bold"])}')
raise HubLoginRequired

def _make_hub_table_with_local(manifests, local_manifests):
Copy link
Member

Choose a reason for hiding this comment

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

I prefer the wording _get

return info_table


def _make_hub_table(manifests):
Copy link
Member

Choose a reason for hiding this comment

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

same here

@deepankarm deepankarm merged commit 6605b2c into master Jan 14, 2021
@deepankarm deepankarm deleted the hub-cleanup branch January 14, 2021 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core This issue/PR affects the core codebase area/helper This issue/PR affects the helper functionality area/testing This issue/PR affects testing component/docker size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants