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.

Sign up for GitHub

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

Jump to bottom

backend: Deployments refactor; Add deployment service and fix deployment config setting #831

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

backend: Deployments refactor; Add deployment service and fix deployment config setting #831

wants to merge 3 commits into from
+8,508 −6,435

Conversation

malexw
Copy link
Collaborator

@malexw malexw commented Nov 6, 2024 •
edited by cohere-pr-pal bot
Loading

The motivation for this change was a request to improve how deployment configs are set in Coral.

Deployments are set up by creating subclasses of BaseDeployment and usually these have the configuration values we want to use when calling models. However, we also have the ability to make changes to these models, which are then stored in the DB. This refactor introduces a deployment_service module to hide the differences between models that are purely defined in code and models that have updates stored in the DB.

This change also finishes off the partially-completed Coral work that allows for changing model config values.

There is a new potential security issue now introduced. We are assuming that anyone who is running Toolkit should have permission to view and change global deployment configurations, which includes API keys. If that's not true, we'll need to add a concept of admin users in the future and prevent non-admins from viewing and changing these values.

AI Description

This PR introduces a new DeploymentNotFoundError exception to handle cases where a deployment is not found. It also refactors the get_deployment function to use a new deployment_service module, which provides methods for retrieving deployments by name or ID. The get_deployment function now returns a BaseDeployment instance, which is a base class for all model deployment options.

The PR also adds a new DeploymentInfo class, which represents the information required to interact with an LLM. This class is used in various parts of the codebase, including the create_deployment, update_deployment, and get_deployment functions.

Additionally, the PR updates the create_deployment_by_config and create_model_by_config functions to use the new DeploymentInfo class. It also updates the validate_deployment_config function to use the new deployment_service module.

The PR also introduces a new get_installed_deployments function, which returns a list of installed deployments. This function is used in the get_available_deployments function, which has been updated to use the new deployment_service module.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also introduces a new update_config function, which updates the configuration of a deployment. This function is used in the set_env_vars function, which has been updated to use the new deployment_service module.

The PR also updates the validate_env_vars function to use the new deployment_service module. This function is used in the create_agent function, which has been updated to use the new DeploymentInfo class.

The PR also introduces a new get_default_deployment function, which returns the default deployment. This function is used in the get_deployment function, which has been updated to use the new deployment_service module.

The PR also updates the get_available_deployments function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployments_info function to use the new deployment_service module. This function is used in the list_deployments function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info function to use the new deployment_service module. This function is used in the get_deployment function, which has been updated to use the new DeploymentInfo class.

The PR also updates the get_deployment_info_by_name function to use the new deployment_service module. This function is used in the get_deployments_info function, which has been updated to use the new DeploymentInfo class.

The PR also updates

malexw
malexw commented Nov 6, 2024
View reviewed changes
Copy link
Collaborator Author

@malexw malexw left a comment

Choose a reason for hiding this comment

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

TODOs

src/backend/cli/main.py Outdated Show resolved Hide resolved
src/backend/config/settings.py Outdated
@@ -372,6 +372,20 @@ def get(self, path: str) -> Any:
return None
return value

def safe_lookup(self, *args, start=None, default=None):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like we have a new get function that is similar; let's use that one instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just got to this point - yup let's use get!

src/backend/crud/deployment.py Outdated Show resolved Hide resolved
src/backend/crud/deployment.py Outdated Show resolved Hide resolved
src/backend/main.py Show resolved Hide resolved
src/backend/schemas/deployment.py Outdated Show resolved Hide resolved
src/backend/services/request_validators.py Outdated Show resolved Hide resolved
src/interfaces/coral_web/.env.development Outdated Show resolved Hide resolved
src/interfaces/coral_web/package.json Outdated Show resolved Hide resolved
src/interfaces/coral_web/src/components/EditEnvVariablesButton.tsx Outdated Show resolved Hide resolved
tianjing-li
tianjing-li requested changes Nov 10, 2024
View reviewed changes
Copy link
Collaborator

@tianjing-li tianjing-li left a comment

Choose a reason for hiding this comment

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

Great great PR, happy to see a lot of these changes. Added some general comments throughout

src/backend/chat/custom/utils.py Show resolved Hide resolved
src/backend/chat/custom/utils.py
if default is not None:
return default
try:
deployment = deployment_service.get_deployment_by_name(name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would the DeploymentNotFoundError trigger if no deployment is found when filtering through the DB?

Perhaps we should use the fallback logic in a:

if not deployment:
    .. get_default_deployment()

And wrap the whole thing in a try/catch instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think I understand what you're seeing. With the new code here, deployment_service will throw if it can't find a deployment with the specified name. In that case, we catch and instead return a default deployment. And if there are no available deployments at all, get_default_deployment will also throw.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I see, I confused the get_deployment_by_name call with a similarly named method I think. Good to go then.

src/backend/cli/main.py Outdated Show resolved Hide resolved
src/backend/config/deployments.py Outdated Show resolved Hide resolved
src/backend/config/deployments.py Outdated Show resolved Hide resolved
src/backend/routers/deployment.py
env_vars: UpdateDeploymentEnv,
valid_env_vars=Depends(validate_env_vars),
ctx: Context = Depends(get_context),
valid_env_vars = Depends(validate_env_vars),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Am good with that. Feel free to scope this in another ticket

src/backend/services/deployment.py Show resolved Hide resolved
src/backend/services/deployment.py
logger = LoggerFactory().get_logger()


def get_default_deployment() -> type[BaseDeployment]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion for the whole file:

I think we can make the code more DRY, my suggestion is maybe adding helper functions to do the iterating portion and then use them, eg

# Helper functions
def _get_first_available_deployment(fallback) -> type[BaseDeployment]:
    return next((d for d in AVAILABLE_MODEL_DEPLOYMENTS if d.is_available), fallback)


... other helper functions to get by name and id

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I took a closer look at these and was able to eliminate one repeated loop, but the others are all just subtly different enough from each other that I don't think there's much common code to extract.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's keep it as is in that case

src/community/config/deployments.py Outdated Show resolved Hide resolved
src/backend/config/deployments.py

def get_installed_deployments() -> list[type[BaseDeployment]]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very small nit to rename get_available_deployments

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Happy to rename this to whatever, but the reason I wanted to get away from the name available is because the models have an is_available method on them, and it might give the impression that a function named get_available_deployments was filtering based on is_available.

tianjing-li
tianjing-li approved these changes Nov 19, 2024
View reviewed changes
Copy link
Collaborator

@tianjing-li tianjing-li left a comment

Choose a reason for hiding this comment

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

Looks good to me, pinged @EugeneLightsOn to help take a second look

EugeneLightsOn
EugeneLightsOn requested changes Nov 20, 2024
View reviewed changes
Copy link
Collaborator

@EugeneLightsOn EugeneLightsOn left a comment

Choose a reason for hiding this comment

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

Great work, some changes are requested

src/backend/database_models/seeders/deplyments_models_seed.py
@@ -18,7 +25,7 @@
model_deployments.update(COMMUNITY_DEPLOYMENTS_SETUP)
Copy link
Collaborator

Choose a reason for hiding this comment

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

After these changes we need to make changes in line 176:
for env_var in model_deployments[deployment].env_vars()

Copy link
Collaborator

Choose a reason for hiding this comment

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

and in line 181, we have no .deployment_class now
]._name_,

src/backend/services/request_validators.py
),
None,
)
if not deployment_model:
deployment_model = model_crud.create_model_by_config(
session, deployment_db, deployment_config, model
session, found, deployment_config, model
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As I see, deployment_config has been removed, and I don’t completely understand why.The idea is that if, for some reason, there is no deployment or model in the database, we attempt to recreate it using a predefined configuration. It was also used during the agent creation flow

src/backend/crud/deployment.py
@@ -193,14 +193,14 @@ def delete_deployment(db: Session, deployment_id: str) -> None:


@validate_transaction
def create_deployment_by_config(db: Session, deployment_config: DeploymentSchema) -> Deployment:
def create_deployment_by_config(db: Session, deployment_config: DeploymentDefinition) -> Deployment:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t see this method being used anymore.

src/backend/services/request_validators.py
raise HTTPException(
status_code=400,
detail=f"Deployment {deployment} not found or is not available in the Database.",
)

if not deployment_db:
deployment_db = deployment_crud.create_deployment_by_config(session, deployment_config)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we no longer need the functionality to create a deployment and model in the database if they were somehow deleted or a new type was added to the configuration?

EugeneLightsOn
EugeneLightsOn requested changes Nov 20, 2024
View reviewed changes
src/backend/crud/deployment.py
@@ -213,7 +213,7 @@ def create_deployment_by_config(db: Session, deployment_config: DeploymentSchema
for env_var in deployment_config.env_vars
},
deployment_class_name=deployment_config.deployment_class.__name__,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure that we have deployment_class attribute for new configuration

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EugeneLightsOn EugeneLightsOn EugeneLightsOn requested changes

@tianjing-li tianjing-li tianjing-li approved these changes

@tomtobac tomtobac Awaiting requested review from tomtobac tomtobac is a code owner

@abimacarmes abimacarmes Awaiting requested review from abimacarmes abimacarmes is a code owner

@knajjars knajjars Awaiting requested review from knajjars knajjars is a code owner

@BeatrixCohere BeatrixCohere Awaiting requested review from BeatrixCohere BeatrixCohere is a code owner

@scottmx81 scottmx81 Awaiting requested review from scottmx81 scottmx81 is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@malexw @tianjing-li @EugeneLightsOn