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

Yaml config sets #2876

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

Yaml config sets #2876

wants to merge 17 commits into from

Conversation

Adam-D-Lewis
Copy link
Member

@Adam-D-Lewis Adam-D-Lewis commented Dec 9, 2024

Reference Issues or PRs

Related to #2865

What does this implement/fix?

Put a x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request locally?
  • Did you add new tests?

How to test this PR?

Run command in #2876 (comment) and see if result matches. Check that nodes are in the expected order: (general, user, worker, then gpu nodes)

Any other comments?

@Adam-D-Lewis
Copy link
Member Author

Adam-D-Lewis commented Dec 9, 2024

Runnnig
nebari init gcp --project-name scratch --domain-name adam.nebari.dev --namespace dev --auth-provider password --ci-provider none --terraform-state local --region us-central1 -s config_set.yaml

with this file:

# config_set.yaml
metadata:
  name: scratch
  nebari_version: ">2024.11.2"
  description: A scratch config set
config:
  google_cloud_platform:
    node_groups:
      gpu-tesla-t4-x1:
        instance: "n1-standard-16"
        min_nodes: 0
        max_nodes: 4
        guest_accelerators:
        - name: nvidia-tesla-t4      # 1x 16 GB GDDR6: Nividia Tesla T4
          count: 1

      gpu-tesla-t4-x2:
        instance: "n1-standard-16"
        min_nodes: 0
        max_nodes: 4
        guest_accelerators:
        - name: nvidia-tesla-t4      # 2x 16 GB GDDR6: Nividia Tesla T4
          count: 2

      gpu-tesla-t4-x4:
        instance: "n1-standard-16"
        min_nodes: 0
        max_nodes: 4
        guest_accelerators:
        - name: nvidia-tesla-t4      # 4x 16 GB GDDR6: Nividia Tesla T4
          count: 4

      gpu-ampere-a100-x1:
        instance: a2-highgpu-1g      # 1x 40 GB HBM2: Nvidia Ampere A100
        min_nodes: 0
        max_nodes: 1

resulted in this nebari config file.

project_name: scratch
google_cloud_platform:
  node_groups:
    gpu-tesla-t4-x1:
      instance: "n1-standard-16"
      min_nodes: 0
      max_nodes: 4
      guest_accelerators:
      - name: nvidia-tesla-t4        # 1x 16 GB GDDR6: Nividia Tesla T4
        count: 1

    general:
      instance: e2-standard-8
      min_nodes: 1
      max_nodes: 1
      preemptible: false
      labels: {}
      guest_accelerators: []
    gpu-tesla-t4-x4:
      instance: "n1-standard-16"
      min_nodes: 0
      max_nodes: 4
      guest_accelerators:
      - name: nvidia-tesla-t4        # 4x 16 GB GDDR6: Nividia Tesla T4
        count: 4

    user:
      instance: e2-standard-4
      min_nodes: 0
      max_nodes: 5
      preemptible: false
      labels: {}
      guest_accelerators: []
    worker:
      instance: e2-standard-4
      min_nodes: 0
      max_nodes: 5
      preemptible: false
      labels: {}
      guest_accelerators: []
    gpu-ampere-a100-x1:
      instance: a2-highgpu-1g        # 1x 40 GB HBM2: Nvidia Ampere A100
      min_nodes: 0
      max_nodes: 1


    gpu-tesla-t4-x2:
      instance: "n1-standard-16"
      min_nodes: 0
      max_nodes: 4
      guest_accelerators:
      - name: nvidia-tesla-t4        # 2x 16 GB GDDR6: Nividia Tesla T4
        count: 2

  kubernetes_version: 1.29.11
  region: us-central1
  project: nebari-dev-testing-omb1
theme:
  jupyterhub:
    hub_title: Nebari - scratch
    welcome: Welcome! Learn about Nebari's features and configurations in <a href="https://www.nebari.dev/docs/welcome">the
      documentation</a>. If you have any questions or feedback, reach the team on
      <a href="https://www.nebari.dev/docs/community#getting-support">Nebari's support
      forums</a>.
    hub_subtitle: Your open source data science platform, hosted on Google Cloud Platform
terraform_state:
  type: local
provider: gcp
domain: adam.nebari.dev
namespace: dev
security:
  keycloak:
    initial_root_password: w4i4zlx7q49269qf9e2isgdm66pk08h3
  authentication:
    type: password
nebari_version: 2024.9.2.dev132+g8e59c242
ci_cd:
  type: none

@Adam-D-Lewis Adam-D-Lewis changed the base branch from main to viniciusdc-patch-test December 9, 2024 23:57
@Adam-D-Lewis Adam-D-Lewis changed the base branch from viniciusdc-patch-test to main December 9, 2024 23:57
@Adam-D-Lewis
Copy link
Member Author

Adam-D-Lewis commented Dec 10, 2024

  • Make the merge keep the original order (new node types after default config node types)
  • add tests for config set
  • fix existing tests

@Adam-D-Lewis Adam-D-Lewis marked this pull request as ready for review December 13, 2024 05:09
@Adam-D-Lewis Adam-D-Lewis added this to the 2024.12.2 release milestone Dec 17, 2024
@Adam-D-Lewis Adam-D-Lewis linked an issue Dec 18, 2024 that may be closed by this pull request

class ConfigSetMetadata(BaseModel):
model_config: ConfigDict = ConfigDict(extra="allow", arbitrary_types_allowed=True)
name: str # for use with guided init
Copy link
Contributor

Choose a reason for hiding this comment

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

If none is allowed here, shouldn't the type be Optional[str] = None?

test_version = "2024.12.2"


def test_valid_version_requirement():
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we parameterize this test with a few different version strings to ensure it works in all the allowed formats? ideally including pre-release versions.

assert metadata.nebari_version.specifier.contains(test_version)


def test_invalid_version_requirement():
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be parameterized too to validate multiple invalid types

Copy link
Contributor

@dcmcand dcmcand left a comment

Choose a reason for hiding this comment

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

Overall, looking good, and does what it says, just a couple of minor issues that I see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Changes requested 🧱
Development

Successfully merging this pull request may close these issues.

3 participants