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: Support communicating with individual Executors in CustomGateway #5558

Merged
merged 35 commits into from
Jan 16, 2023

Conversation

Jackmin801
Copy link
Contributor

@Jackmin801 Jackmin801 commented Dec 27, 2022

Goals:

@github-actions github-actions bot added size/S area/core This issue/PR affects the core codebase labels Dec 27, 2022
@Jackmin801
Copy link
Contributor Author

Jackmin801 commented Dec 27, 2022

Gateway code:

from docarray import DocumentArray, Document

from jina.serve.runtimes.gateway.http.fastapi import FastAPIBaseGateway
from jina import Flow, Executor, requests

class MyGateway(FastAPIBaseGateway):
    @property
    def app(self):
        from fastapi import FastAPI

        app = FastAPI(title='Custom FastAPI Gateway')

        @app.get("/endpoint")
        async def get(text: str):
            docs = await self.executor['executor1'](exec_endpoint='/', docs=DocumentArray([Document(text=text)]), parameters={'k': 'v'})
            return {'result': docs[0].text}

        return app


class FirstExec(Executor):
    @requests
    def func(self, docs, **kwargs):
        for doc in docs:
            doc.text += " First"

class SecondExec(Executor):
    @requests
    def func(self, docs, **kwargs):
        print(kwargs)
        for doc in docs:
            doc.text += " Second"

with Flow(port=12345).config_gateway(uses=MyGateway, protocol='http', port=50975).add(
    uses=FirstExec, name='executor0'
).add(
    uses=SecondExec, name='executor1'
) as flow:
    flow.block()

Client code

curl 'localhost:50975/endpoint?text=meow'

@codecov
Copy link

codecov bot commented Dec 27, 2022

Codecov Report

Merging #5558 (73a94b6) into master (a5ef69b) will decrease coverage by 15.58%.
The diff coverage is 33.33%.

@@             Coverage Diff             @@
##           master    #5558       +/-   ##
===========================================
- Coverage   86.17%   70.59%   -15.59%     
===========================================
  Files         124      122        -2     
  Lines       10048    10022       -26     
===========================================
- Hits         8659     7075     -1584     
- Misses       1389     2947     +1558     
Flag Coverage Δ
jina 70.59% <33.33%> (-15.59%) ⬇️

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

Impacted Files Coverage Δ
jina/serve/streamer.py 77.65% <26.31%> (-13.01%) ⬇️
jina/serve/gateway.py 92.18% <100.00%> (-3.06%) ⬇️
jina/schemas/flow.py 0.00% <0.00%> (-100.00%) ⬇️
jina/schemas/meta.py 0.00% <0.00%> (-100.00%) ⬇️
jina/schemas/gateway.py 0.00% <0.00%> (-100.00%) ⬇️
jina/schemas/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
jina/schemas/executor.py 0.00% <0.00%> (-100.00%) ⬇️
jina/schemas/deployment.py 0.00% <0.00%> (-100.00%) ⬇️
jina/schemas/helper.py 0.00% <0.00%> (-91.67%) ⬇️
jina/checker.py 0.00% <0.00%> (-85.72%) ⬇️
... and 69 more

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

@Jackmin801 Jackmin801 changed the title feat: Support communicating with individual Executors with GatewayStreamer feat: Support communicating with individual Executors in CustomGateway Dec 27, 2022
@Jackmin801 Jackmin801 marked this pull request as draft December 27, 2022 08:03
@github-actions github-actions bot added the area/testing This issue/PR affects testing label Dec 27, 2022
jina/serve/streamer.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added size/M area/docs This issue/PR affects the docs labels Dec 27, 2022
docs/concepts/gateway/customization.md Outdated Show resolved Hide resolved
docs/concepts/gateway/customization.md Outdated Show resolved Hide resolved
docs/concepts/gateway/customization.md Outdated Show resolved Hide resolved
docs/concepts/gateway/customization.md Outdated Show resolved Hide resolved
jina/serve/streamer.py Outdated Show resolved Hide resolved
jina/serve/streamer.py Outdated Show resolved Hide resolved
tests/unit/serve/gateway/test_gateway.py Outdated Show resolved Hide resolved
jina/serve/streamer.py Outdated Show resolved Hide resolved
Co-authored by: Joan Fontanals <joan.martinez@jina.ai>
Copy link
Contributor

@alaeddine-13 alaeddine-13 left a comment

Choose a reason for hiding this comment

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

I suggest 2 options:

  • keep introducing ExecutorStreamer, I think in this case it makes sense to rename GatewayStreamer to FlowStreamer. Since we are making a breaking change, it makes sense to unify the interface as well. This means having:
  • self.flow_streamer.post(...) # or self.flow.post(...)
  • self.executor_streamer[exec].post(...) # or self.executors[exec].post(...)
  • unify flow streamer and executor streamer under the same self.streamer object. I think this makes sense because otherwise, since the same pool is used, destroying one object corrupts the other

docs/concepts/gateway/customization.md Outdated Show resolved Hide resolved
tests/unit/serve/gateway/test_gateway.py Outdated Show resolved Hide resolved
tests/unit/serve/gateway/test_gateway.py Outdated Show resolved Hide resolved
tests/unit/serve/gateway/test_gateway.py Outdated Show resolved Hide resolved
@Jackmin801 Jackmin801 marked this pull request as ready for review January 4, 2023 08:24
@JoanFM JoanFM closed this Jan 10, 2023
@JoanFM JoanFM reopened this Jan 10, 2023
Jackmin801 and others added 3 commits January 10, 2023 17:00
Signed-off-by: Jackmin801 <56836461+Jackmin801@users.noreply.github.com>
Signed-off-by: Jackmin801 <56836461+Jackmin801@users.noreply.github.com>
@Jackmin801 Jackmin801 closed this Jan 10, 2023
@Jackmin801
Copy link
Contributor Author

zz bot

@Jackmin801 Jackmin801 reopened this Jan 10, 2023
@github-actions github-actions bot removed the area/cli This issue/PR affects the command line interface label Jan 11, 2023
@@ -221,3 +221,36 @@ def get_streamer():
@staticmethod
def _set_env_streamer_args(**kwargs):
os.environ['JINA_STREAMER_ARGS'] = json.dumps(kwargs)


class _ExecutorStreamer:
Copy link
Member

Choose a reason for hiding this comment

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

no, I do not think so

tests/unit/serve/gateway/test_gateway.py Outdated Show resolved Hide resolved
@Jackmin801 Jackmin801 force-pushed the feat-5538-comm-with-individual-execs branch from 37c68c9 to 504e9cb Compare January 11, 2023 11:49
this makes the tests more resilient and more likely to hit all the
replicas
@Jackmin801 Jackmin801 force-pushed the feat-5538-comm-with-individual-execs branch from 504e9cb to d1348e4 Compare January 11, 2023 11:49
JoanFM
JoanFM previously approved these changes Jan 16, 2023
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.

Technically approve, wait for @alexcg1 for the docs

docs/concepts/gateway/customization.md Outdated Show resolved Hide resolved
docs/concepts/gateway/customization.md Outdated Show resolved Hide resolved
docs/concepts/gateway/customization.md Outdated Show resolved Hide resolved
Co-authored-by: Alex Cureton-Griffiths <alexcg1@users.noreply.github.com>
Copy link
Member

@alexcg1 alexcg1 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@github-actions
Copy link

📝 Docs are deployed on https://feat-5538-comm-with-individual-execs--jina-docs.netlify.app 🎉

@JoanFM JoanFM merged commit e875424 into master Jan 16, 2023
@JoanFM JoanFM deleted the feat-5538-comm-with-individual-execs branch January 16, 2023 11:48
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/docs This issue/PR affects the docs area/testing This issue/PR affects testing size/M size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support communicating with individual Executors with GatewayStreamer
5 participants