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

Add support for conditional prompting in bundle init #971

Merged
merged 16 commits into from
Nov 30, 2023

Conversation

shreyas-goenka
Copy link
Contributor

Changes

This PR introduces the skip_prompt_if extension to the jsonschema library. If the inputs provided by the user match the JSON schema then the prompt for that property is skipped.

Right now only constant checks are supported, but if in the future more complicated conditionals are required, this can be extended to support allOf, oneOf, anyOf etc allowing template authors to specify conditionals of arbitary complexity.

Tests

Unit tests and manually.

@shreyas-goenka
Copy link
Contributor Author

shreyas-goenka commented Nov 8, 2023

Manual testing example. Note here question two will be skipped if answer to question one is no:

Schema:

{
  "properties": {
    "question_1": {
      "order": 1,
      "type": "string",
      "description": "\nFirst question",
      "default": "yes",
      "enum": ["yes", "no"]
    },
    "question_2": {
      "order": 2,
      "type": "string",
      "default": "a-default-value",
      "description": "\nSecond question. Only visible if you answered 'yes' to the first question.",
      "skip_prompt_if": {
        "properties": {
          "question_1": {
            "const": "no"
          }
        }
      }
    },
    "question_3": {
      "order": 3,
      "type": "string",
      "description": "\nThird question"
    }
  }
}

Prompts:

shreyas.goenka@THW32HFW6T mlops-stack % cli bundle init ~/mlops-stack

First question: yes

Second question. Only visible if you answered 'yes' to the first question. [a-default-value]: 

Third question: ^C
shreyas.goenka@THW32HFW6T mlops-stack % cli bundle init ~/mlops-stack

First question: no

Third question: ^C

Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

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

Nice work. The extensions are growing and we should be careful adding more.

libs/jsonschema/extension.go Outdated Show resolved Hide resolved
libs/jsonschema/schema.go Show resolved Hide resolved
libs/jsonschema/schema_test.go Show resolved Hide resolved
libs/template/config.go Show resolved Hide resolved
libs/template/config.go Show resolved Hide resolved
libs/template/config.go Show resolved Hide resolved
libs/template/config_test.go Show resolved Hide resolved
libs/template/config.go Outdated Show resolved Hide resolved
@shreyas-goenka
Copy link
Contributor Author

The skipPrompt function still includes value mutation inside it. This is to make unit testing easier.

Expanded the scope of the function to consider execution of default value as a string and skip assigning default value if the property already has a value.

Note: The abstractions here are kind of messy. We need to combine the config and the renderer structs. I'll work on a followup to do so once this is merged.

Properties: map[string]*Schema{
"foo": {
Type: "string",
Const: "abc",
Copy link
Contributor

Choose a reason for hiding this comment

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

What are valid types for Const? Only strings and integers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Const can be any type, including null or objects according to the JSON schema spec: https://json-schema.org/draft/2020-12/json-schema-validation#name-const

Copy link
Contributor Author

@shreyas-goenka shreyas-goenka Nov 29, 2023

Choose a reason for hiding this comment

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

For mlops-stacks, we only the capability to validate const strings right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@shreyas-goenka what if I define the following const, does it work?

"properties": {
        "def": {
            "type": "float",
            "const": 5.1
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried it out. Does not work for float because of precision issues. (5.1 vs 5.099).

Also does not work for integers due to type not being correct (basically const is parsed as a float rather than an int). I can fix it in a followup PR.

@shreyas-goenka shreyas-goenka added this pull request to the merge queue Nov 30, 2023
Merged via the queue into main with commit bdef0f7 Nov 30, 2023
4 checks passed
@shreyas-goenka shreyas-goenka deleted the conditional-prompts branch November 30, 2023 16:13
andrewnester added a commit that referenced this pull request Dec 6, 2023
CLI:
 * Add documentation for positional args in commands generated from the Databricks OpenAPI specification ([#1033](#1033)).
 * Ask for host when .databrickscfg doesn't exist ([#1041](#1041)).
 * Add list of supported values for flags that represent an enum field ([#1036](#1036)).

Bundles:
 * Fix panic when bundle auth resolution fails ([#1002](#1002)).
 * Add versioning for bundle templates ([#972](#972)).
 * Add support for conditional prompting in bundle init ([#971](#971)).
 * Pass parameters to task when run with `--python-params` and `python_wheel_wrapper` is true ([#1037](#1037)).
 * Change default_python template to auto-update version on each wheel build ([#1034](#1034)).

Internal:
 * Rewrite the friendly log handler ([#1038](#1038)).
 * Move bundle schema update to an internal module ([#1012](#1012)).

Dependency updates:
 * Bump github.com/databricks/databricks-sdk-go from 0.26.0 to 0.26.1 ([#1040](#1040)).
@andrewnester andrewnester mentioned this pull request Dec 6, 2023
github-merge-queue bot pushed a commit that referenced this pull request Dec 6, 2023
CLI:
* Add documentation for positional args in commands generated from the
Databricks OpenAPI specification
([#1033](#1033)).
* Ask for host when .databrickscfg doesn't exist
([#1041](#1041)).
* Add list of supported values for flags that represent an enum field
([#1036](#1036)).

Bundles:
* Fix panic when bundle auth resolution fails
([#1002](#1002)).
* Add versioning for bundle templates
([#972](#972)).
* Add support for conditional prompting in bundle init
([#971](#971)).
* Pass parameters to task when run with `--python-params` and
`python_wheel_wrapper` is true
([#1037](#1037)).
* Change default_python template to auto-update version on each wheel
build ([#1034](#1034)).

Internal:
* Rewrite the friendly log handler
([#1038](#1038)).
* Move bundle schema update to an internal module
([#1012](#1012)).


Dependency updates:
* Bump github.com/databricks/databricks-sdk-go from 0.26.0 to 0.26.1
([#1040](#1040)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants