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

fix: post should handle version properly #441

Merged
merged 5 commits into from
Jul 15, 2022
Merged

fix: post should handle version properly #441

merged 5 commits into from
Jul 15, 2022

Conversation

bwanglzu
Copy link
Collaborator

@bwanglzu bwanglzu commented Jul 14, 2022

Context

Not a big fan of the solution, but the current implementation of post is not usable:

  1. versions are not handled. e.g. executor versions such as v0.9.1-gpu is commonly used while using an executor. The current implementation only allow us to use default, which is latest. This result in host parsing not correctly parse the executor versions.
  2. keyword arguments are not parsed into Executor, such as uses_with, volumes.

This could be better handled by unifying jina.hubble.helper.parse_hub_uri, or even better to create an extra on argument to the post method, but it will introduce breaking change, so i did not implement it.

It is already working. But i would do a bit more refactoring, move the logic of urlparsing out of post, to make it unit testable.

Why this is needed?

from docarray import DocumentArray, Document

da = DocumentArray([Document(text='hello')])
da = da.post(
    'jinahub+docker://FinetunerExecutor/v0.9.2/encode',
    uses_with={'artifact': '/mnt/finetune-quora-19.zip'},
    volumes=['/Users/bo/Documents/work2/test/my-model/:/mnt'],
)
assert da[0].embedding.shape==(768,)

@codecov
Copy link

codecov bot commented Jul 14, 2022

Codecov Report

Merging #441 (07fef96) into main (25a59af) will increase coverage by 1.69%.
The diff coverage is 96.77%.

@@            Coverage Diff             @@
##             main     #441      +/-   ##
==========================================
+ Coverage   84.93%   86.62%   +1.69%     
==========================================
  Files         134      134              
  Lines        6466     6477      +11     
==========================================
+ Hits         5492     5611     +119     
+ Misses        974      866     -108     
Flag Coverage Δ
docarray 86.62% <96.77%> (+1.69%) ⬆️

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

Impacted Files Coverage Δ
docarray/array/mixins/post.py 93.02% <96.77%> (+5.52%) ⬆️
docarray/array/mixins/find.py 87.35% <0.00%> (+1.14%) ⬆️
docarray/helper.py 82.80% <0.00%> (+1.35%) ⬆️
docarray/array/storage/sqlite/seqlike.py 84.31% <0.00%> (+1.96%) ⬆️
docarray/array/queryset/lookup.py 78.10% <0.00%> (+2.18%) ⬆️
docarray/array/mixins/io/binary.py 97.45% <0.00%> (+2.54%) ⬆️
docarray/array/storage/weaviate/seqlike.py 77.41% <0.00%> (+3.22%) ⬆️
docarray/array/mixins/io/from_gen.py 83.63% <0.00%> (+3.63%) ⬆️
docarray/array/storage/weaviate/backend.py 87.50% <0.00%> (+3.67%) ⬆️
docarray/array/storage/elastic/backend.py 95.04% <0.00%> (+4.13%) ⬆️
... and 9 more

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 25a59af...07fef96. Read the comment docs.

@bwanglzu bwanglzu marked this pull request as ready for review July 15, 2022 07:59
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 not instead of pattern matching version just check if there are more / than expected, and u just extract the last part as the endpoint, and everything before as the host where u apply some extra logic?

@bwanglzu
Copy link
Collaborator Author

@JoanFM do you think check extra / is even more fragile?

@JoanFM
Copy link
Member

JoanFM commented Jul 15, 2022

@JoanFM do you think check extra / is even more fragile?

it is much easier to mantain

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.

LGTM, please make sure it is clear in documentation

@JoanFM JoanFM merged commit d16f10e into main Jul 15, 2022
@JoanFM JoanFM deleted the fix-post-endpoint branch July 15, 2022 09:34
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.

None yet

2 participants