-
Notifications
You must be signed in to change notification settings - Fork 67
[GCP] Update artifact registry/db code #685
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
Open
AaDalal
wants to merge
15
commits into
main
Choose a base branch
from
aagamdalal/sgp-3576-model-engine-update-artifact-registry-model-engine-code
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 12 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
8db5a37
Add google-cloud-artifact-registry dep
AaDalal 8d8d8fa
Add gcp_artifact_registry_docker_repository
AaDalal 7894737
[untested] completed gcp_artifact_registry_docker_repository
AaDalal f19da2b
Add TODO comments in places I don't understand
AaDalal 0655ccd
[deps] Bump protobuf and add google-secret-manager
AaDalal c2993f8
Creating a model bundle hits artifact registry!
AaDalal 66d4f3a
Remove todo comments
AaDalal 5d4c90c
Remove gcp_project_id infra config key in favor of reusing ml_account_id
AaDalal 04f5a19
Use redis for task queue gateway and cache
AaDalal 06df969
Add redis on GCP support
AaDalal 8c6265f
Document values_sample.yaml better
AaDalal 3bd84cf
Add documentation on how to build llm-engine docker images to README
AaDalal 777a604
Clean up todos
AaDalal d2019e5
Cleanup more todos
AaDalal a7074ca
Return image tag instead of image name in
AaDalal 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
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
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,3 +1,4 @@ | ||
| import json | ||
| import os | ||
| import sys | ||
| import time | ||
|
|
@@ -7,6 +8,7 @@ | |
| import sqlalchemy | ||
| from azure.identity import DefaultAzureCredential | ||
| from azure.keyvault.secrets import SecretClient | ||
| from google.cloud.secretmanager_v1 import SecretManagerServiceClient | ||
| from model_engine_server.core.aws.secrets import get_key_file | ||
| from model_engine_server.core.config import InfraConfig, infra_config | ||
| from model_engine_server.core.loggers import logger_name, make_logger | ||
|
|
@@ -20,8 +22,12 @@ | |
|
|
||
|
|
||
| def get_key_file_name(environment: str) -> str: | ||
| if infra_config().cloud_provider == "azure": | ||
| # azure and gcp don't support "/" in the key file secret name | ||
| # so we use dashes | ||
| if infra_config().cloud_provider == "azure" or infra_config().cloud_provider == "gcp": | ||
| return f"{environment}-ml-infra-pg".replace("training", "prod").replace("-new", "") | ||
|
|
||
| # aws does support "/" in the key file secret name | ||
| return f"{environment}/ml_infra_pg".replace("training", "prod").replace("-new", "") | ||
|
|
||
|
|
||
|
|
@@ -55,16 +61,17 @@ def get_engine_url( | |
| key_file = os.environ.get("DB_SECRET_NAME") | ||
| if env is None: | ||
| env = infra_config().env | ||
|
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. What are the values of env / where is it used? |
||
| # TODO: what are the values of env? | ||
| if key_file is None: | ||
| key_file = get_key_file_name(env) # type: ignore | ||
| logger.debug(f"Using key file {key_file}") | ||
|
|
||
| if infra_config().cloud_provider == "azure": | ||
| client = SecretClient( | ||
| az_secret_client = SecretClient( | ||
| vault_url=f"https://{os.environ.get('KEYVAULT_NAME')}.vault.azure.net", | ||
| credential=DefaultAzureCredential(), | ||
| ) | ||
| db = client.get_secret(key_file).value | ||
| db = az_secret_client.get_secret(key_file).value | ||
| user = os.environ.get("AZURE_IDENTITY_NAME") | ||
| token = DefaultAzureCredential().get_token( | ||
| "https://ossrdbms-aad.database.windows.net/.default" | ||
|
|
@@ -76,6 +83,28 @@ def get_engine_url( | |
| # for recommendations on how to work with rotating auth credentials | ||
| engine_url = f"postgresql://{user}:{password}@{db}?sslmode=require" | ||
| expiry_in_sec = token.expires_on | ||
| elif infra_config().cloud_provider == "gcp": | ||
| gcp_secret_manager_client = ( | ||
| SecretManagerServiceClient() | ||
| ) # uses application default credentials (see: https://cloud.google.com/secret-manager/docs/reference/libraries#client-libraries-usage-python) | ||
| secret_version = gcp_secret_manager_client.access_secret_version( | ||
| request={ | ||
| "name": f"projects/{infra_config().ml_account_id}/secrets/{key_file}/versions/latest" | ||
| } | ||
| ) | ||
| creds = json.loads(secret_version.payload.data.decode("utf-8")) | ||
|
|
||
| user = creds.get("username") | ||
| password = creds.get("password") | ||
| host = creds.get("host") | ||
| port = str(creds.get("port")) | ||
| dbname = creds.get("dbname") | ||
|
|
||
| assert all([user, password, host, port, dbname]) # TODO: remove this | ||
|
|
||
| logger.info(f"Connecting to db {host}:{port}, name {dbname}") | ||
|
|
||
| engine_url = f"postgresql://{user}:{password}@{host}:{port}/{dbname}" | ||
| else: | ||
| db_secret_aws_profile = os.environ.get("DB_SECRET_AWS_PROFILE") | ||
| creds = get_key_file(key_file, db_secret_aws_profile) | ||
|
|
||
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kovben95scale had a good question about this -- is there a reason we don't use redis on azure etc? I think we're just using this as a celery task queue here, which seems like it would fit with redis.