-
Notifications
You must be signed in to change notification settings - Fork 1.2k
perf: Add init and cleanup of long lived resources #4642
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
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
87d59fe
rebase
robhowley 7793d99
offline store init doesnt make sense
robhowley f04711e
dont init or close
robhowley d1094ad
update test to handle event loop for dynamo case
robhowley afaa412
use run util complete
robhowley 2245caa
fix: spelling sigh
robhowley eebf71d
run integration test as async since that is default for read
robhowley e593c1a
add pytest async to ci reqs
robhowley f50c6cb
be safe w cleanup in test fixture
robhowley 5c4a88a
be safe w cleanup in test fixture
robhowley 92dc4bd
update pytest ini
robhowley ee2249a
not in a finally
robhowley 5c69273
remove close
robhowley 385e3ea
test client is a lifespan aware context manager
robhowley File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,11 +12,13 @@ | |
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| import asyncio | ||
| import contextlib | ||
| import itertools | ||
| import logging | ||
| from datetime import datetime | ||
| from typing import Any, Callable, Dict, List, Literal, Optional, Sequence, Tuple, Union | ||
|
|
||
| from aiobotocore.config import AioConfig | ||
| from pydantic import StrictBool, StrictStr | ||
|
|
||
| from feast import Entity, FeatureView, utils | ||
|
|
@@ -75,6 +77,9 @@ class DynamoDBOnlineStoreConfig(FeastConfigBaseModel): | |
| session_based_auth: bool = False | ||
| """AWS session based client authentication""" | ||
|
|
||
| max_pool_connections: int = 10 | ||
| """Max number of connections for async Dynamodb operations""" | ||
|
|
||
|
|
||
| class DynamoDBOnlineStore(OnlineStore): | ||
| """ | ||
|
|
@@ -87,7 +92,14 @@ class DynamoDBOnlineStore(OnlineStore): | |
|
|
||
| _dynamodb_client = None | ||
| _dynamodb_resource = None | ||
| _aioboto_session = None | ||
|
|
||
| async def initialize(self, config: RepoConfig): | ||
| await _get_aiodynamodb_client( | ||
| config.online_store.region, config.online_store.max_pool_connections | ||
| ) | ||
|
|
||
| async def close(self): | ||
| await _aiodynamodb_close() | ||
|
|
||
| @property | ||
| def async_supported(self) -> SupportedAsyncMethods: | ||
|
|
@@ -326,15 +338,17 @@ def to_tbl_resp(raw_client_response): | |
| batches.append(batch) | ||
| entity_id_batches.append(entity_id_batch) | ||
|
|
||
| async with self._get_aiodynamodb_client(online_config.region) as client: | ||
| response_batches = await asyncio.gather( | ||
| *[ | ||
| client.batch_get_item( | ||
| RequestItems=entity_id_batch, | ||
| ) | ||
| for entity_id_batch in entity_id_batches | ||
| ] | ||
| ) | ||
| client = await _get_aiodynamodb_client( | ||
| online_config.region, online_config.max_pool_connections | ||
| ) | ||
|
Comment on lines
-329
to
+343
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we're now reusing the client and doing context management via |
||
| response_batches = await asyncio.gather( | ||
| *[ | ||
| client.batch_get_item( | ||
| RequestItems=entity_id_batch, | ||
| ) | ||
| for entity_id_batch in entity_id_batches | ||
| ] | ||
| ) | ||
|
|
||
| result_batches = [] | ||
| for batch, response in zip(batches, response_batches): | ||
|
|
@@ -349,14 +363,6 @@ def to_tbl_resp(raw_client_response): | |
|
|
||
| return list(itertools.chain(*result_batches)) | ||
|
|
||
| def _get_aioboto_session(self): | ||
| if self._aioboto_session is None: | ||
| self._aioboto_session = session.get_session() | ||
| return self._aioboto_session | ||
|
|
||
| def _get_aiodynamodb_client(self, region: str): | ||
| return self._get_aioboto_session().create_client("dynamodb", region_name=region) | ||
|
|
||
| def _get_dynamodb_client( | ||
| self, | ||
| region: str, | ||
|
|
@@ -489,6 +495,38 @@ def _to_client_batch_get_payload(online_config, table_name, batch): | |
| } | ||
|
|
||
|
|
||
| _aioboto_session = None | ||
| _aioboto_client = None | ||
|
|
||
|
|
||
| def _get_aioboto_session(): | ||
| global _aioboto_session | ||
| if _aioboto_session is None: | ||
| logger.debug("initializing the aiobotocore session") | ||
| _aioboto_session = session.get_session() | ||
| return _aioboto_session | ||
|
|
||
|
|
||
| async def _get_aiodynamodb_client(region: str, max_pool_connections: int): | ||
| global _aioboto_client | ||
| if _aioboto_client is None: | ||
| logger.debug("initializing the aiobotocore dynamodb client") | ||
| client_context = _get_aioboto_session().create_client( | ||
| "dynamodb", | ||
| region_name=region, | ||
| config=AioConfig(max_pool_connections=max_pool_connections), | ||
| ) | ||
| context_stack = contextlib.AsyncExitStack() | ||
| _aioboto_client = await context_stack.enter_async_context(client_context) | ||
| return _aioboto_client | ||
|
|
||
|
|
||
| async def _aiodynamodb_close(): | ||
| global _aioboto_client | ||
| if _aioboto_client: | ||
| await _aioboto_client.close() | ||
|
|
||
|
|
||
| def _initialize_dynamodb_client( | ||
| region: str, | ||
| endpoint_url: Optional[str] = None, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,6 @@ | ||
| [pytest] | ||
| asyncio_mode = auto | ||
|
|
||
| markers = | ||
| universal_offline_stores: mark a test as using all offline stores. | ||
| universal_online_stores: mark a test as using all online stores. | ||
|
|
@@ -7,6 +9,8 @@ env = | |
| IS_TEST=True | ||
|
|
||
| filterwarnings = | ||
| error::_pytest.warning_types.PytestConfigWarning | ||
| error::_pytest.warning_types.PytestUnhandledCoroutineWarning | ||
|
Comment on lines
+12
to
+13
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fail if any async test functions are skipped bc of missing plugins |
||
| ignore::DeprecationWarning:pyspark.sql.pandas.*: | ||
| ignore::DeprecationWarning:pyspark.sql.connect.*: | ||
| ignore::DeprecationWarning:httpx.*: | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.