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

feat: pass arbitrary kwargs to docker sdk #1690

Merged
merged 2 commits into from Jan 15, 2021
Merged

feat: pass arbitrary kwargs to docker sdk #1690

merged 2 commits into from Jan 15, 2021

Conversation

JoanFM
Copy link
Member

@JoanFM JoanFM commented Jan 14, 2021

Changes introduced
As requested in #1671 , adding arbitrary parameters can be useful to get custom behavior or options for the docker images supporting Peas living in Docker images

@JoanFM JoanFM requested a review from a team as a code owner January 14, 2021 15:35
@JoanFM JoanFM marked this pull request as draft January 14, 2021 15:35
@jina-bot jina-bot added size/S area/core This issue/PR affects the core codebase area/network This issue/PR affects network functionality component/peapod labels Jan 14, 2021
@tadejsv
Copy link
Contributor

tadejsv commented Jan 14, 2021

Oh it was this easy 😂

@JoanFM
Copy link
Member Author

JoanFM commented Jan 14, 2021

Oh it was this easy

Have to test still, so not sure it was this easy, just a draft

@codecov
Copy link

codecov bot commented Jan 14, 2021

Codecov Report

Merging #1690 (0c3e633) into master (122fdf4) will increase coverage by 0.62%.
The diff coverage is 81.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1690      +/-   ##
==========================================
+ Coverage   84.45%   85.07%   +0.62%     
==========================================
  Files         132      133       +1     
  Lines        6830     6848      +18     
==========================================
+ Hits         5768     5826      +58     
+ Misses       1062     1022      -40     
Impacted Files Coverage Δ
jina/docker/checker.py 84.90% <ø> (+2.76%) ⬆️
jina/flow/base.py 87.56% <ø> (ø)
jina/executors/indexers/keyvalue.py 98.68% <66.66%> (-1.32%) ⬇️
jina/docker/hubio.py 70.27% <74.35%> (+7.00%) ⬆️
jina/docker/hubapi/remote.py 75.32% <75.32%> (ø)
jina/parsers/helper.py 51.92% <80.00%> (+5.25%) ⬆️
jina/parsers/hub/__init__.py 65.00% <80.00%> (-0.12%) ⬇️
jina/docker/hubapi/local.py 81.70% <81.70%> (ø)
jina/drivers/control.py 95.08% <85.71%> (-1.35%) ⬇️
jina/drivers/cache.py 94.11% <100.00%> (+0.78%) ⬆️
... 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 122fdf4...ba76a55. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Jan 14, 2021

Latency summary

Current PR yields:

  • 😶 index QPS at 1594, delta to last 3 avg.: +2%
  • 😶 query QPS at 31, delta to last 3 avg.: -3%

Breakdown

Version Index QPS Query QPS
current 1594 31
0.9.13 1608 32
0.9.12 1503 31

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

@jina-bot jina-bot added the area/testing This issue/PR affects testing label Jan 14, 2021
@JoanFM JoanFM marked this pull request as ready for review January 14, 2021 16:26
@tadejsv
Copy link
Contributor

tadejsv commented Jan 14, 2021

Would you be able to add a test for using this in a yaml config as well?

@JoanFM
Copy link
Member Author

JoanFM commented Jan 14, 2021

Would you be able to add a test for using this in a yaml config as well?

I am thinking it won't look very nice, I am trying to see how can I pass direct dictionaries to argparser

@tadejsv
Copy link
Contributor

tadejsv commented Jan 14, 2021

Would you be able to add a test for using this in a yaml config as well?

I am thinking it won't look very nice, I am trying to see how can I pass direct dictionaries to argparser

I also came across this problem when thinking how to implement this yesterday. I think the way you did it is probably the best way currently - argparser is unfortunately very limited - you could use something like eval, but it's use is discouraged.

A more fundamental solution would require moving away from Argparser, but that would take a long time

@JoanFM
Copy link
Member Author

JoanFM commented Jan 14, 2021

Would you be able to add a test for using this in a yaml config as well?

I am thinking it won't look very nice, I am trying to see how can I pass direct dictionaries to argparser

I also came across this problem when thinking how to implement this yesterday. I think the way you did it is probably the best way currently - argparser is unfortunately very limited - you could use something like eval, but it's use is discouraged.

A more fundamental solution would require moving away from Argparser, but that would take a long time

I found a quite good solution with a helper we are already using.

To use it in the yaml files, the syntax would be similar to the usage of env in Flow.yml as it can be seen in:

tests/integration/optimizers/flow.yml

@tadejsv
Copy link
Contributor

tadejsv commented Jan 14, 2021

Would you be able to add a test for using this in a yaml config as well?

I am thinking it won't look very nice, I am trying to see how can I pass direct dictionaries to argparser

I also came across this problem when thinking how to implement this yesterday. I think the way you did it is probably the best way currently - argparser is unfortunately very limited - you could use something like eval, but it's use is discouraged.
A more fundamental solution would require moving away from Argparser, but that would take a long time

I found a quite good solution with a helper we are already using.

To use it in the yaml files, the syntax would be similar to the usage of env in Flow.yml as it can be seen in:

tests/integration/optimizers/flow.yml

Pretty cool :) I guess for now it is not possible to pass list/dict arguments, right? I mean the following [but through command line/yaml]

docker_kwargs = {'environment': ['VAR1=BAR', 'VAR2=FOO']}

@JoanFM
Copy link
Member Author

JoanFM commented Jan 14, 2021

Would you be able to add a test for using this in a yaml config as well?

I am thinking it won't look very nice, I am trying to see how can I pass direct dictionaries to argparser

I also came across this problem when thinking how to implement this yesterday. I think the way you did it is probably the best way currently - argparser is unfortunately very limited - you could use something like eval, but it's use is discouraged.
A more fundamental solution would require moving away from Argparser, but that would take a long time

I found a quite good solution with a helper we are already using.
To use it in the yaml files, the syntax would be similar to the usage of env in Flow.yml as it can be seen in:
tests/integration/optimizers/flow.yml

Pretty cool :) I guess for now it is not possible to pass list/dict arguments, right? I mean the following [but through command line/yaml]

docker_kwargs = {'environment': ['VAR1=BAR', 'VAR2=FOO']}

Not sure, should be tested If u want u can add a tesr and do a PR to this branch or I will try to test tmr

@tadejsv
Copy link
Contributor

tadejsv commented Jan 14, 2021

I can try testing it a bit later yes

@JoanFM
Copy link
Member Author

JoanFM commented Jan 15, 2021

Hey @tadejsv , I did some changes to allow passing strings and arbitrary values, plus I added a test to showcase and document how it can be passed into a yaml

@tadejsv
Copy link
Contributor

tadejsv commented Jan 15, 2021

Great work @JoanFM !!

daemon/models/custom.py Outdated Show resolved Hide resolved
@JoanFM JoanFM merged commit cc619f9 into master Jan 15, 2021
@JoanFM JoanFM deleted the docker-kwargs branch January 15, 2021 10:33
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/network This issue/PR affects network functionality area/testing This issue/PR affects testing component/flow component/peapod size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants