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: add specific executor param #4934

Closed
wants to merge 4 commits into from
Closed

Conversation

samsja
Copy link
Contributor

@samsja samsja commented Jun 16, 2022

Context

ATM when you pass a parameters to an Executor client.index(docs, parameters = {...} every Executor will receive all of them.
Meaning that if you want to pass different parameters to each Executor you will need to change the name.That's problematic because some Executor except the same parameters, like traversal_path.

For instance you can't embed on the chunk with one executor and embed on the root with an other one because you can only pass one traversal_path.

We need to be able to send parameters to different Executor.

original issue here : #4919
this PR take over the old proposition which we decide not to go for : #4923

Proposal for a Solution

Using the key__executor_name instead of key to precise that it should only be sent to the given Executor.

The __ operator is use for this kind of things in the python world. We use it in docarray : da[:,'tags__key'].

This is actually the exact same syntax of scikit-learn pipeline.

Parameters of the estimators in the pipeline can be accessed using the <estimator>__<parameter> syntax

from jina import Flow

f = Flow().add(name='exec1').add(name='exec2')

f.post(parameters= {'key1_exec1': val1}' # like this Exec1 will receive {'key1': val1} in their params and exec2 will receive empty.
f.post(parameters= {'key1': val1}' # like this Exec1  and Exec2 will receive {'key1': val1} 

Moreover : regarding performance issue of deserialization on the gateway side I suggest to avoid doing any deserialization on the gateway side by sending the full parameters to the Worker and only passing the one relevant to the Executor

What this PR do:

  • Allow to use the __ syntax to pass executor to one Executor
  • tests
  • documentation

What still need to be fix/done:

  • find a better way to retrieve the original name of the Executor
  • better pasring machanism, something like __results__ or __something__ should not be considerate while parsing

@github-actions github-actions bot added size/M area/core This issue/PR affects the core codebase area/testing This issue/PR affects testing labels Jun 16, 2022
@codecov
Copy link

codecov bot commented Jun 16, 2022

Codecov Report

Merging #4934 (004a874) into master (1cd8645) will decrease coverage by 2.74%.
The diff coverage is 97.05%.

@@            Coverage Diff             @@
##           master    #4934      +/-   ##
==========================================
- Coverage   87.86%   85.12%   -2.75%     
==========================================
  Files         111      111              
  Lines        8763     8794      +31     
==========================================
- Hits         7700     7486     -214     
- Misses       1063     1308     +245     
Flag Coverage Δ
jina 85.12% <97.05%> (-2.68%) ⬇️

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

Impacted Files Coverage Δ
.../runtimes/request_handlers/data_request_handler.py 90.68% <97.05%> (-3.94%) ⬇️
jina/jaml/parsers/flow/v1.py 63.15% <0.00%> (-35.09%) ⬇️
jina/orchestrate/flow/base.py 71.91% <0.00%> (-18.34%) ⬇️
jina/jaml/parsers/__init__.py 75.60% <0.00%> (-17.08%) ⬇️
jina/serve/executors/__init__.py 75.41% <0.00%> (-11.18%) ⬇️
jina/serve/runtimes/gateway/http/__init__.py 90.19% <0.00%> (-7.85%) ⬇️
jina/serve/runtimes/gateway/websocket/__init__.py 90.19% <0.00%> (-7.85%) ⬇️
jina/serve/executors/decorators.py 91.66% <0.00%> (-6.25%) ⬇️
...a/orchestrate/deployments/config/docker_compose.py 94.55% <0.00%> (-5.45%) ⬇️
... and 20 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 6d7811d...004a874. Read the comment docs.

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.

for the logic u mentioned it should be 'executorname__param` and not the other way around right?

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.

I am not sure this solution can be handled. Since the name of the Executor w.r.t the Flow is not known by the Deployment. Imagine an External Executor cannot have the same name for every Flow

:param executor_name: name of the Executor
:returns: the parsed parameters after applying filtering for the specific Executor
"""
parsed_params = copy.deepcopy(parameters)
Copy link
Member

Choose a reason for hiding this comment

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

no need to copy

Copy link
Member

Choose a reason for hiding this comment

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

this is already a dict generated from protobuf

@samsja samsja mentioned this pull request Jun 20, 2022
4 tasks
@JoanFM JoanFM closed this Jun 20, 2022
@JoanFM JoanFM deleted the feat-param-per-executor branch July 20, 2023 15:17
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/testing This issue/PR affects testing size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants