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: run warmup on Runtimes and Executor #5579

Merged
merged 40 commits into from Jan 14, 2023
Merged

Conversation

girishc13
Copy link
Contributor

@girishc13 girishc13 commented Jan 6, 2023

Goals:

  • resolves feat: run warmup requests on runtime startup to ensure that service is ready to accept connections #5467
  • Gateway and Head runtime now starts a warmup tasks on the run_forever runtime method.
  • The warmup task is created by the GatewayStreamer or the Head RequestHandler that delegates the warmup per deployments to the connection pool.
    • A single task per deployment is created and await until completion.
  • The GrpcConnectionPool fetches the available shards and or replica (target) channels and creates a JinaInfoRPC request task.
    • One task is created for each channel.
    • The warmup cycle runs until:
      • all targets return a successful response or
      • a max timeout of 5 minutes is reached or
      • a stop event is set by the runtime due to early tear down of the runtime.
    • In the warmup cycle, only unsuccessful targets are attempted.
  • CompositeGateway doesn't support the warmup due to the creation of separate GatewayStreamer objects for each type of Gateway. The issue will be addressed in feat: use single GatewayStreamer object for all gateways in the CompositeGateway #5592.
    • the CompositeGateway methods shutdown, run_server, setup_server now handle each gateway operation as a task per created gateway.

@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 Jan 6, 2023
@girishc13 girishc13 force-pushed the feat-serve-5467-runtime-warmup branch from 7622872 to 5817aac Compare January 6, 2023 15:18
@girishc13 girishc13 closed this Jan 6, 2023
@girishc13 girishc13 reopened this Jan 6, 2023
@codecov
Copy link

codecov bot commented Jan 6, 2023

Codecov Report

Merging #5579 (6232b42) into master (d70ec53) will increase coverage by 1.87%.
The diff coverage is 89.56%.

@@            Coverage Diff             @@
##           master    #5579      +/-   ##
==========================================
+ Coverage   86.11%   87.98%   +1.87%     
==========================================
  Files         124      124              
  Lines        9939    10047     +108     
==========================================
+ Hits         8559     8840     +281     
+ Misses       1380     1207     -173     
Flag Coverage Δ
jina 87.98% <89.56%> (+1.87%) ⬆️

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

Impacted Files Coverage Δ
jina/serve/runtimes/worker/request_handling.py 96.17% <ø> (+5.53%) ⬆️
jina/serve/runtimes/head/request_handling.py 90.78% <66.66%> (-3.33%) ⬇️
jina/serve/runtimes/asyncio.py 85.82% <78.57%> (+2.64%) ⬆️
jina/serve/streamer.py 93.33% <82.35%> (-3.28%) ⬇️
jina/serve/networking.py 90.45% <94.33%> (+1.93%) ⬆️
jina/serve/runtimes/gateway/__init__.py 98.36% <100.00%> (+0.08%) ⬆️
jina/serve/runtimes/gateway/composite/gateway.py 100.00% <100.00%> (ø)
jina/serve/runtimes/head/__init__.py 97.85% <100.00%> (+0.11%) ⬆️
jina/serve/runtimes/worker/__init__.py 95.74% <100.00%> (+7.88%) ⬆️
jina/orchestrate/flow/base.py 90.08% <0.00%> (+0.12%) ⬆️
... and 27 more

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

@girishc13 girishc13 closed this Jan 9, 2023
@girishc13 girishc13 reopened this Jan 9, 2023
@github-actions github-actions bot added the area/cli This issue/PR affects the command line interface label Jan 9, 2023
@girishc13 girishc13 force-pushed the feat-serve-5467-runtime-warmup branch from 22acbb4 to 280d481 Compare January 10, 2023 16:47
@github-actions github-actions bot removed the area/cli This issue/PR affects the command line interface label Jan 11, 2023

def __extract_target_to_channel(self, deployment):
replica_set = set()
replica_set.update(self._connections.get_replicas_all_shards(deployment))
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 can extract out the warmup logic into a Warmer class but the self._connections is the inner class _ConnectionPoolMap of the GrpcConnectionPool. Does the inner class still make sense?

@girishc13 girishc13 closed this Jan 13, 2023
@girishc13 girishc13 reopened this Jan 13, 2023
@github-actions github-actions bot added the area/cli This issue/PR affects the command line interface label Jan 13, 2023
@github-actions github-actions bot removed the area/cli This issue/PR affects the command line interface label Jan 13, 2023
@girishc13 girishc13 marked this pull request as ready for review January 13, 2023 13:45
@@ -1,6 +1,8 @@
import asyncio
import ipaddress
import os
import threading
import time
Copy link
Member

Choose a reason for hiding this comment

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

please remove unneeded imports

@@ -1,6 +1,7 @@
import argparse
import asyncio
import signal
import threading
Copy link
Member

Choose a reason for hiding this comment

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

remove unneeded import

jina/serve/runtimes/head/request_handling.py Outdated Show resolved Hide resolved
# create a new set of stubs and channels for warmup to avoid
# loosing channel during remove_connection or reset_connection
stubs, _ = self._create_connection(address, deployment_name)
self._warmup_stubs.add(stubs)
Copy link
Member

Choose a reason for hiding this comment

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

I do not see where they are used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check the usage of the property method self.warmup_stubs. What's the preference when exposing read only properties?

Copy link
Member

Choose a reason for hiding this comment

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

ah okey

@JoanFM JoanFM merged commit ed9b448 into master Jan 14, 2023
@JoanFM JoanFM deleted the feat-serve-5467-runtime-warmup branch January 14, 2023 10:14
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.

feat: run warmup requests on runtime startup to ensure that service is ready to accept connections
3 participants