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

Explicit config #2294

Merged
merged 150 commits into from
Jun 17, 2024
Merged

Explicit config #2294

merged 150 commits into from
Jun 17, 2024

Conversation

Adam-D-Lewis
Copy link
Member

@Adam-D-Lewis Adam-D-Lewis commented Mar 6, 2024

Reference Issues or PRs

Fixes #2122, based on #2348 which should be merged first

Currently, we set all the nebari-config fields manually in

def render_config(

but it's confusing for users to not see some useful parts of the config (e.g. #2122). We could add more fields to render_config method, but every time we add a new field in any InputSchema, we'd have to update the render_config method which is inconvenient. Additionally, having the fields hard coded in render_config means Nebari extensions don't have their InputSchema's written to the config file even if installed. They would need to be added some other way (e.g. typing them in manually) in order to control the extension's settings exposed by it's InputSchema.

This PR instead writes all of the InputSchema's to the nebari-config file by default, but this led to some confusing config in cases like infrastructure InputSchema where this PR caused a section to be added for each of "local", "existing", "google_cloud_platform", etc. for each cloud prodiver and all except one of these provider sections was blank.

class InputSchema(schema.Base):
local: Optional[LocalProvider]
existing: Optional[ExistingProvider]
google_cloud_platform: Optional[GoogleCloudPlatformProvider]
amazon_web_services: Optional[AmazonWebServicesProvider]
azure: Optional[AzureProvider]
digital_ocean: Optional[DigitalOceanProvider]

So this PR also allows InputSchemas to define a exclude_from_config method which returns the names of the fields to exclude from the config file to fix this issue. We likely could address this issue by separating the different providers into separated Pydantic Models (similar to what I've done to the certificate Pydantic models), but I feel it's important to have some mechanism to manually exclude portions of the InputSchema from being written to the nebari-config file when running nebari init, and have to chosen to address the extra providers with exclude_from_config.

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): nebari init results in a more complete nebari-config.yaml

Testing

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

Any other comments?

pyproject.toml Outdated
"PTH",
"E", # E: pycodestyle rules
"F", # F: pyflakes rules
"PTH", # PTH: flake8-use-pathlib rules
]
ignore = [
"E501", # Line too long
"F821", # Undefined name
"PTH123", # open() should be replaced by Path.open()
]
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes a deprecation warning from ruff.

@Adam-D-Lewis Adam-D-Lewis changed the base branch from develop to upgrade-grafana June 4, 2024 21:17
@Adam-D-Lewis Adam-D-Lewis changed the base branch from upgrade-grafana to develop June 4, 2024 21:17
@Adam-D-Lewis Adam-D-Lewis changed the title Explicit config [WIP] Explicit config Jun 4, 2024
@Adam-D-Lewis
Copy link
Member Author

Adam-D-Lewis commented Jun 4, 2024

Okay, I've really paired down this PR to just show all the config explicitly and nothing else. It is going to be confusing for beginners, and in some spots even for more advanced users (the certificate section mashing all the different certificates together comes to mind).

certificate:
  type: self-signed
  secret_name:
  acme_email:
  acme_server: https://acme-v02.api.letsencrypt.org/directory

type: self-signed is from self signed certs, secret_name is for existing certs. acme email and server are for lets encrypt certs only.

I'd like to merge anyway. The idea is that this is just for advanced users, and if it's heavily used and desired, we can work on making it less confusing iteratively. We invite users to open issues where clarity is desired. For reference, an explict config generated from running nebari init -p scratch-2efc --domain github-actions.nebari.dev --verbose looks like the following:

project_name: scratch-2efc
namespace: dev
provider: local
nebari_version: 2024.6.1rc3.dev14+gbb8c8789
prevent_deploy: false
ci_cd:
  type: none
  branch: main
  commit_render: true
  before_script: []
  after_script: []
terraform_state:
  type: remote
  backend:
  config: {}
local:
  kube_context:
  node_selectors:
    general:
      key: kubernetes.io/os
      value: linux
    user:
      key: kubernetes.io/os
      value: linux
    worker:
      key: kubernetes.io/os
      value: linux
existing:
google_cloud_platform:
amazon_web_services:
azure:
digital_ocean:
external_container_reg:
  enabled: false
  access_key_id:
  secret_access_key:
  extcr_account:
  extcr_region:
domain: github-actions.nebari.dev
certificate:
  type: self-signed
  secret_name:
  acme_email:
  acme_server: https://acme-v02.api.letsencrypt.org/directory
ingress:
  terraform_overrides: {}
dns:
  provider:
  auto_provision: false
security:
  authentication:
    type: password
  shared_users_group: true
  keycloak:
    initial_root_password: 437ijez00hmc5jg1fam7hsyvkfibftzx
    overrides: {}
    realm_display_name: Nebari
default_images:
  jupyterhub: quay.io/nebari/nebari-jupyterhub:2024.5.1
  jupyterlab: quay.io/nebari/nebari-jupyterlab:2024.5.1
  dask_worker: quay.io/nebari/nebari-dask-worker:2024.5.1
storage:
  conda_store: 200Gi
  shared_filesystem: 200Gi
theme:
  jupyterhub:
    hub_title: Nebari - scratch-2efc
    hub_subtitle: Your open source data science platform, hosted
    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>.
    logo: 
      https://raw.githubusercontent.com/nebari-dev/nebari-design/main/logo-mark/horizontal/Nebari-Logo-Horizontal-Lockup-White-text.svg
    favicon: 
      https://raw.githubusercontent.com/nebari-dev/nebari-design/main/symbol/favicon.ico
    primary_color: '#4f4173'
    primary_color_dark: '#4f4173'
    secondary_color: '#957da6'
    secondary_color_dark: '#957da6'
    accent_color: '#32C574'
    accent_color_dark: '#32C574'
    text_color: '#111111'
    h1_color: '#652e8e'
    h2_color: '#652e8e'
    version: v2024.6.1rc3.dev14+gbb8c8789
    navbar_color: '#1c1d26'
    navbar_text_color: '#f1f1f6'
    navbar_hover_color: '#db96f3'
    display_version: 'True'
profiles:
  jupyterlab:
  - access: all
    display_name: Small Instance
    description: Stable environment with 2 cpu / 8 GB ram
    default: true
    users:
    groups:
    kubespawner_override:
      cpu_limit: 2.0
      cpu_guarantee: 1.5
      mem_limit: 8G
      mem_guarantee: 5G
  - access: all
    display_name: Medium Instance
    description: Stable environment with 4 cpu / 16 GB ram
    default: false
    users:
    groups:
    kubespawner_override:
      cpu_limit: 4.0
      cpu_guarantee: 3.0
      mem_limit: 16G
      mem_guarantee: 10G
  dask_worker:
    Small Worker:
      worker_cores_limit: 2.0
      worker_cores: 1.5
      worker_memory_limit: 8G
      worker_memory: 5G
      worker_threads: 2
    Medium Worker:
      worker_cores_limit: 4.0
      worker_cores: 3.0
      worker_memory_limit: 16G
      worker_memory: 10G
      worker_threads: 4
environments:
  environment-dask.yaml:
    name: dask
    channels:
    - conda-forge
    dependencies:
    - python==3.11.6
    - ipykernel==6.26.0
    - ipywidgets==8.1.1
    - nebari-dask==2024.5.1
    - python-graphviz==0.20.1
    - pyarrow==14.0.1
    - s3fs==2023.10.0
    - gcsfs==2023.10.0
    - numpy=1.26.0
    - numba=0.58.1
    - pandas=2.1.3
    - xarray==2023.10.1
  environment-dashboard.yaml:
    name: dashboard
    channels:
    - conda-forge
    dependencies:
    - python==3.11.6
    - cufflinks-py==0.17.3
    - dash==2.14.1
    - geopandas==0.14.1
    - geopy==2.4.0
    - geoviews==1.11.0
    - gunicorn==21.2.0
    - holoviews==1.18.1
    - ipykernel==6.26.0
    - ipywidgets==8.1.1
    - jupyter==1.0.0
    - jupyter_bokeh==3.0.7
    - matplotlib==3.8.1
    - nebari-dask==2024.5.1
    - nodejs=20.8.1
    - numpy==1.26.0
    - openpyxl==3.1.2
    - pandas==2.1.3
    - panel==1.3.1
    - param==2.0.1
    - plotly==5.18.0
    - python-graphviz==0.20.1
    - rich==13.6.0
    - streamlit==1.28.1
    - sympy==1.12
    - voila==0.5.5
    - xarray==2023.10.1
    - pip==23.3.1
    - pip:
      - streamlit-image-comparison==0.0.4
      - noaa-coops==0.1.9
      - dash_core_components==2.0.0
      - dash_html_components==2.0.0
conda_store:
  extra_settings: {}
  extra_config: ''
  image: quansight/conda-store-server
  image_tag: 2024.3.1
  default_namespace: nebari-git
  object_storage: 200Gi
argo_workflows:
  enabled: true
  overrides: {}
  nebari_workflow_controller:
    enabled: true
    image_tag: 2024.5.1
monitoring:
  enabled: true
  overrides:
    loki: {}
    promtail: {}
    minio: {}
  minio_enabled: true
telemetry:
  jupyterlab_pioneer:
    enabled: false
    log_format:
jupyterhub:
  overrides: {}
jupyterlab:
  default_settings: {}
  idle_culler:
    terminal_cull_inactive_timeout: 15
    terminal_cull_interval: 5
    kernel_cull_idle_timeout: 15
    kernel_cull_interval: 5
    kernel_cull_connected: true
    kernel_cull_busy: false
    server_shutdown_no_activity_timeout: 15
  initial_repositories: []
  preferred_dir:
jhub_apps:
  enabled: false
helm_extensions: []
tf_extensions: []

@Adam-D-Lewis Adam-D-Lewis added needs: review 👀 This PR is complete and ready for reviewing and removed status: in progress 🏗 This task is currently being worked on labels Jun 13, 2024
@Adam-D-Lewis
Copy link
Member Author

I'll rename verbose to explicit after talking with Marcelo

@Adam-D-Lewis
Copy link
Member Author

I'll check if there any applicable tests to add as well

Copy link
Member

@marcelovilla marcelovilla left a comment

Choose a reason for hiding this comment

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

This is great @Adam-D-Lewis 🚀 ! Thank you!

Everything works as expected so I'm approving. Here are a couple comments for discussion but feel free to ignore them if you don't find them relevant.

  • See comment regarding having the explicit option being an int instead of a bool.
  • I noticed that on the guided init, the explicit config prompt is only shown when the users confirms they want to make advanced configuration changes. While I agree this might be an option for "advanced" users, I feel it might be a little bit hidden under that section. However, I don't have any strong feelings as one can always use the --explicit flag so I think if we document it well enough, it should not be a problem.

@@ -565,6 +566,13 @@ def init(
"-o",
help="Output file path for the rendered config file.",
),
explicit: int = typer.Option(
Copy link
Member

Choose a reason for hiding this comment

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

@Adam-D-Lewis is there any reason to have int over bool here? I know either of those will work but I'm wondering if there is any advantage.

Copy link
Member Author

Choose a reason for hiding this comment

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

Making it an int allows you to specify --explicit --explicit or -ee to have levels of explicitness. We aren't using the levels yet, but may in the future.

@Adam-D-Lewis
Copy link
Member Author

I noticed that on the guided init, the explicit config prompt is only shown when the users confirms they want to make advanced configuration changes. While I agree this might be an option for "advanced" users, I feel it might be a little bit hidden under that section. However, I don't have any strong feelings as one can always use the --explicit flag so I think if we document it well enough, it should not be a problem.

I think it's going to be confusing for users until we document it better (as done here) so I think it is an advanced configuration setting that should be hidden there for the moment. Happy to move it once it's better documented.

@Adam-D-Lewis Adam-D-Lewis merged commit cf0f754 into develop Jun 17, 2024
29 checks passed
@Adam-D-Lewis Adam-D-Lewis deleted the explicit_config branch June 17, 2024 19:19
@Adam-D-Lewis
Copy link
Member Author

Thanks for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: review 👀 This PR is complete and ready for reviewing
Projects
Development

Successfully merging this pull request may close these issues.

[BUG] - Config generated by guided init should contain default settings
6 participants