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

chore: Change CLI and constants #8593

Merged
merged 3 commits into from
Dec 14, 2023
Merged

chore: Change CLI and constants #8593

merged 3 commits into from
Dec 14, 2023

Conversation

szewaiyuen6
Copy link
Contributor

@szewaiyuen6 szewaiyuen6 commented Dec 13, 2023

Description

Change user-facing parts of the det deploy that references lore to genai.

Internal references clean up will be a separate task (lower pri).

Test Plan

det deploy aws up --cluster-id swy-lore-test4 --region us-west-2 --keypair szewai-determined-aws --compute-agent-instance-type g4dn.12xlarge --max-dynamic-agents 10 --det-v 115f685a4cd4025118a198751a81b5e0105c5f68 --max-idle-agent-period 60m --deployment-type genai --genai-version a45d508

Screen Shot 2023-12-13 at 12 57 29 PM Screen Shot 2023-12-13 at 12 58 04 PM

Commentary (optional)

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

Ticket

Copy link

netlify bot commented Dec 13, 2023

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 115f685
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/657b4d5bd4bd1d0009734614

@szewaiyuen6 szewaiyuen6 marked this pull request as draft December 13, 2023 19:49
@szewaiyuen6 szewaiyuen6 changed the title Chore: Change CLI and constants chore: Change CLI and constants Dec 13, 2023
@szewaiyuen6 szewaiyuen6 marked this pull request as ready for review December 13, 2023 22:38
Copy link
Member

@hamidzr hamidzr left a comment

Choose a reason for hiding this comment

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

thanks for working on this.

leaving the internal references to lore as lore for as a separate tasks sgtm but I think we should at least migration all the lore.?version references together in this pr otherwise it'd be hard to track

(determined) hbp ➜ determined git:(update_deploy_2) ag 'lore.?version'
harness/determined/deploy/aws/deployment_types/vpc.py
49:        lore_tag = self.parameters[constants.cloudformation.LORE_VERSION] or "latest"

harness/determined/deploy/aws/constants.py
78:    LORE_VERSION = "LoreVersion"

harness/determined/deploy/aws/cli.py
270:        constants.cloudformation.LORE_VERSION: args.genai_version,

harness/determined/deploy/aws/templates/lore.yaml
219:  LoreVersion:
221:    Description: Lore version to deploy
294:      {{ if ne .lore_version "" }}
922:            lore_version: ${LoreVersion}
974:            if [ ! -z "${LoreVersion}" ]; then
983:                --env LORE_DOCKER_TAG_SUFFIX="${LoreVersion}" \
987:                "determinedai/environments-dev:lore-backend-image-${LoreVersion}"

harness/determined/deploy/aws/deployment_types/base.py
45:    constants.cloudformation.LORE_VERSION,

harness/determined/deploy/aws/templates/fsx.yaml
289:      {{ if ne .lore_version "" }}

harness/determined/deploy/aws/templates/simple.yaml
280:      {{ if ne .lore_version "" }}

harness/determined/deploy/aws/templates/efs.yaml
289:      {{ if ne .lore_version "" }}

harness/determined/deploy/aws/templates/govcloud.yaml
234:      {{ if ne .lore_version "" }}

harness/determined/deploy/aws/templates/simple-rds.yaml
288:      {{ if ne .lore_version "" }}

harness/determined/deploy/aws/templates/secure.yaml
284:      {{ if ne .lore_version "" }}

harness/determined/deploy/aws/injector/master.yaml.tmpl
60:{{ if ne .lore_version "" }}
(determined) hbp ➜ determined git:(update_deploy_2)

Copy link
Member

@hamidzr hamidzr left a comment

Choose a reason for hiding this comment

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

lgtm assuming it deploys fine.

we probably want to consider this env var user facing but it needs to be updated on the other side
harness/determined/deploy/aws/templates/lore.yaml:983: --env LORE_DOCKER_TAG_SUFFIX="${GenAIVersion}" \

@@ -42,10 +42,12 @@ class EFS(VPCBase):

class Lore(VPCBase):
Copy link
Contributor

Choose a reason for hiding this comment

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

should this classname be changed too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an internal reference, we are doing the naming migration in phases and focusing on user facing noes first.

Copy link
Contributor

@azhou-determined azhou-determined left a comment

Choose a reason for hiding this comment

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

lgtm

@szewaiyuen6 szewaiyuen6 merged commit 0f5a16d into main Dec 14, 2023
71 of 82 checks passed
@szewaiyuen6 szewaiyuen6 deleted the update_deploy_2 branch December 14, 2023 22:04
@dannysauer dannysauer added this to the 0.27.0 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants