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: expand executor serve parameters #5494

Merged
merged 13 commits into from
Dec 14, 2022
Merged

Conversation

alaeddine-13
Copy link
Contributor

feat: expand executor serve parameters

@github-actions github-actions bot added size/M area/core This issue/PR affects the core codebase labels Dec 6, 2022
@alaeddine-13 alaeddine-13 force-pushed the feat-executor-serve-expand-args branch from c4b4047 to c28ab9e Compare December 6, 2022 11:57
@codecov
Copy link

codecov bot commented Dec 6, 2022

Codecov Report

Merging #5494 (74bdb5d) into master (fd37a09) will increase coverage by 1.94%.
The diff coverage is 96.21%.

@@            Coverage Diff             @@
##           master    #5494      +/-   ##
==========================================
+ Coverage   85.58%   87.53%   +1.94%     
==========================================
  Files         120      121       +1     
  Lines        9410     9648     +238     
==========================================
+ Hits         8054     8445     +391     
+ Misses       1356     1203     -153     
Flag Coverage Δ
jina 87.53% <96.21%> (+1.94%) ⬆️

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

Impacted Files Coverage Δ
jina/clients/mixin.py 97.41% <ø> (ø)
jina/orchestrate/flow/base.py 90.07% <ø> (+6.20%) ⬆️
jina/schemas/helper.py 91.66% <0.00%> (+91.66%) ⬆️
jina/jaml/parsers/executor/legacy.py 88.46% <50.00%> (-1.54%) ⬇️
jina/clients/base/grpc.py 90.42% <90.90%> (-0.25%) ⬇️
jina/serve/executors/decorators.py 96.92% <94.54%> (-1.80%) ⬇️
jina/serve/runtimes/worker/request_handling.py 96.17% <98.07%> (+1.37%) ⬆️
jina/serve/runtimes/worker/batch_queue.py 98.78% <98.78%> (ø)
jina/__init__.py 67.03% <100.00%> (ø)
jina/jaml/__init__.py 95.00% <100.00%> (+0.41%) ⬆️
... and 34 more

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

@alaeddine-13 alaeddine-13 marked this pull request as ready for review December 7, 2022 08:16
@JoanFM
Copy link
Member

JoanFM commented Dec 7, 2022

Wait for #5488 since it touches a little bit

@@ -526,6 +709,7 @@ def serve(
uses_with=uses_with,
uses_metas=uses_metas,
uses_requests=uses_requests,
reload=reload,
)
Copy link
Member

Choose a reason for hiding this comment

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

are u sure all these kwargs are passed to the Executor itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried out the reload locally
Putting the executor definition and executor.serve() in the same file is not supported so I added a warning each time we attempt to reload the main module. In other cases it works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kwargs are passed to the Flow. That way, the user of serve will have full power over everything (gateway + executor).
I confirm that kwargs are passed correctly to Flow and the Flow in return, passes them to gateway and executor (beisdes the ones explicitly passed to the executors)

@alaeddine-13 alaeddine-13 reopened this Dec 7, 2022
@JoanFM JoanFM marked this pull request as draft December 12, 2022 17:21
@JoanFM JoanFM marked this pull request as ready for review December 14, 2022 08:44
@JoanFM JoanFM merged commit a5b9df5 into master Dec 14, 2022
@JoanFM JoanFM deleted the feat-executor-serve-expand-args branch December 14, 2022 08:44
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 size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants