-
Notifications
You must be signed in to change notification settings - Fork 74
Update transpile
to support --overrides-path
and --target-technology
arguments
#2072
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
Changes from all commits
9024730
7225971
aeb6c3f
cbdec92
f1fea8c
aacadf2
da249c5
af75dd0
3dbab6f
5a1eb0e
734d469
77e733f
070dd38
13cd9ed
ec0f36b
ae3077c
79e9d40
0c711c3
96b1925
a95f4c2
10ba183
9b5738d
f280424
5d50103
a600885
b79a94a
52b9fd0
a65907f
ff56327
b6e7793
39a35b3
a1f6db4
268b31e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,14 +16,14 @@ | |
|
||
from databricks.labs.blueprint.cli import App | ||
from databricks.labs.blueprint.entrypoint import get_logger, is_in_debug | ||
from databricks.labs.blueprint.installation import RootJsonValue | ||
from databricks.labs.blueprint.installation import RootJsonValue, JsonObject, JsonValue | ||
from databricks.labs.blueprint.tui import Prompts | ||
|
||
|
||
from databricks.labs.lakebridge.assessments.configure_assessment import create_assessment_configurator | ||
from databricks.labs.lakebridge.assessments import PROFILER_SOURCE_SYSTEM | ||
|
||
from databricks.labs.lakebridge.config import TranspileConfig | ||
from databricks.labs.lakebridge.config import TranspileConfig, LSPConfigOptionV1 | ||
from databricks.labs.lakebridge.contexts.application import ApplicationContext | ||
from databricks.labs.lakebridge.helpers.recon_config_utils import ReconConfigPrompts | ||
from databricks.labs.lakebridge.helpers.telemetry_utils import make_alphanum_or_semver | ||
|
@@ -84,26 +84,33 @@ def _remove_warehouse(ws: WorkspaceClient, warehouse_id: str): | |
|
||
|
||
@lakebridge.command | ||
def transpile( | ||
def transpile( # pylint: disable=too-many-arguments | ||
*, | ||
w: WorkspaceClient, | ||
transpiler_config_path: str | None = None, | ||
source_dialect: str | None = None, | ||
overrides_file: str | None = None, | ||
target_technology: str | None = None, | ||
input_source: str | None = None, | ||
output_folder: str | None = None, | ||
error_file_path: str | None = None, | ||
skip_validation: str | None = None, | ||
catalog_name: str | None = None, | ||
schema_name: str | None = None, | ||
ctx: ApplicationContext | None = None, | ||
asnare marked this conversation as resolved.
Show resolved
Hide resolved
|
||
transpiler_repository: TranspilerRepository = TranspilerRepository.user_home(), | ||
): | ||
"""Transpiles source dialect to databricks dialect""" | ||
ctx = ApplicationContext(w) | ||
if ctx is None: | ||
ctx = ApplicationContext(w) | ||
del w | ||
logger.debug(f"Preconfigured transpiler config: {ctx.transpile_config!r}") | ||
ctx.add_user_agent_extra("cmd", "execute-transpile") | ||
checker = _TranspileConfigChecker(ctx.transpile_config, ctx.prompts, transpiler_repository) | ||
checker.use_transpiler_config_path(transpiler_config_path) | ||
checker.use_source_dialect(source_dialect) | ||
checker.use_overrides_file(overrides_file) | ||
checker.use_target_technology(target_technology) | ||
checker.use_input_source(input_source) | ||
checker.use_output_folder(output_folder) | ||
checker.use_error_file_path(error_file_path) | ||
|
@@ -213,6 +220,36 @@ def use_source_dialect(self, source_dialect: str | None) -> None: | |
logger.debug(f"Pending source_dialect override: {source_dialect!r}") | ||
self._source_dialect_override = source_dialect | ||
|
||
@staticmethod | ||
def _validate_overrides_file(overrides_file: str, msg: str) -> None: | ||
"""Validate the overrides file: it must be a valid path that exists.""" | ||
# Note: in addition to this check, later we verify the transpiler supports it. | ||
if not Path(overrides_file).exists(): | ||
raise_validation_exception(msg) | ||
|
||
def use_overrides_file(self, overrides_file: str | None) -> None: | ||
if overrides_file is not None: | ||
logger.debug(f"Setting overrides_file to: {overrides_file!r}") | ||
msg = f"Invalid path for '--overrides-file', does not exist: {overrides_file}" | ||
self._validate_overrides_file(overrides_file, msg) | ||
try: | ||
self._set_config_transpiler_option("overrides-file", overrides_file) | ||
except ValueError: | ||
# TODO: Update the `config.yml` format to disallow incompatible `transpiler_options`. | ||
msg = "Cannot use --overrides-file; workspace config.yml has incompatible transpiler_options." | ||
raise_validation_exception(msg) | ||
|
||
def use_target_technology(self, target_technology: str | None) -> None: | ||
if target_technology is not None: | ||
logger.debug(f"Setting target_technology to: {target_technology!r}") | ||
# Cannot validate this here: depends on the transpiler engine, and will be checked later. | ||
try: | ||
self._set_config_transpiler_option("target-tech", target_technology) | ||
except ValueError: | ||
# TODO: Update the `config.yml` format to disallow incompatible `transpiler_options`. | ||
asnare marked this conversation as resolved.
Show resolved
Hide resolved
|
||
msg = "Cannot use --target-technology; workspace config.yml has incompatible transpiler_options." | ||
raise_validation_exception(msg) | ||
|
||
@staticmethod | ||
def _validate_input_source(input_source: str, msg: str) -> None: | ||
"""Validate the input source: it must be a path that exists.""" | ||
|
@@ -317,6 +354,19 @@ def use_schema_name(self, schema_name: str | None) -> None: | |
logger.debug(f"Setting schema_name to: {schema_name!r}") | ||
self._config = dataclasses.replace(self._config, schema_name=schema_name) | ||
|
||
def _set_config_transpiler_option(self, flag: str, value: str) -> None: | ||
transpiler_options: JsonObject | ||
match self._config.transpiler_options: | ||
case None: | ||
transpiler_options = {flag: value} | ||
case Mapping() as found_options: | ||
transpiler_options = {**found_options, flag: value} | ||
case found_options: | ||
# TODO: Update `config.yml' to constrain `transpiler_options` to be a dict[str, str]. | ||
msg = f"Incompatible transpiler options configured, must be a mapping: {found_options!r}" | ||
raise ValueError(msg) | ||
self._config = dataclasses.replace(self._config, transpiler_options=transpiler_options) | ||
|
||
def _configure_transpiler_config_path(self, source_dialect: str) -> TranspileEngine | None: | ||
"""Configure the transpiler config path based on the requested source dialect.""" | ||
# Names of compatible transpiler engines for the given dialect. | ||
|
@@ -444,18 +494,52 @@ def _check_transpiler_options(self, engine: TranspileEngine) -> None: | |
assert self._config.source_dialect is not None, "Source dialect must be set before checking transpiler options." | ||
options_for_dialect = engine.options_for_dialect(self._config.source_dialect) | ||
transpiler_options = self._config.transpiler_options | ||
if not isinstance(transpiler_options, Mapping): | ||
return | ||
if transpiler_options is None: | ||
transpiler_options = {} | ||
elif not isinstance(transpiler_options, Mapping): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we properly type this since we know what it looks like instead of passing around Maps e.g.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't really know what it looks like though: it's filled in during the questionnaire based on a configuration file bundled with each LSP server. It certainly can be more constrained than This PR adds specific support for 2 known options, but there might be others passed via this mechanism. (The code is supposed to prompt for missing values, but #2086 describes how this is futile and leads to the CLI simply hanging.) |
||
logger.warning(f"Ignoring transpiler_options in config.yml, must be a mapping: {transpiler_options!r}") | ||
transpiler_options = {} | ||
# Only checks if the option is present, does not validate the value. | ||
# TODO: Validate the value for CHOICE/CONFIRM options. | ||
# TODO: Handle FORCE options: these are fixed by the transpiler, and cannot be overridden. | ||
checked_options = { | ||
option.flag: ( | ||
transpiler_options[option.flag] | ||
if option.flag in transpiler_options | ||
else option.prompt_for_value(self._prompts) | ||
else self._handle_missing_transpiler_option(option) | ||
) | ||
for option in options_for_dialect | ||
} | ||
self._config = dataclasses.replace(self._config, transpiler_options=checked_options) | ||
|
||
def _handle_missing_transpiler_option(self, option: LSPConfigOptionV1) -> JsonValue: | ||
# Semantics during configuration: | ||
# - Entries are present in the config file for all options the LSP server needs for a dialect. | ||
# - If a value is `None`, it means the user wants the value to be left unset. | ||
# - There is no 'provide it later' option: either it's set, or it's unset. | ||
# As a corner case, if there is no entry present it means the user wasn't prompted. Here we have | ||
# some complexity. We have two ways of obtaining a value: | ||
# - The user could provide it on the command-line, using --target-technology or --overrides-file. | ||
# Problem: via command-line options there's no way to indicate 'no value'. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not the way the code was authored, and configuration files are already out there. |
||
# - We could prompt for it, assuming the user is running interactively. | ||
# In terms of what is required by the option: | ||
# - If the option has a default of <none>, it means that no value is required. | ||
# - Everything else requires a value. | ||
# | ||
# This leads to the following business rules: | ||
# - If the option has a default of <none> that means that no value is required, no further action is required. | ||
# - Otherwise, a value is required: prompt for it. | ||
# | ||
# TODO: When adding non-interactive support, the otherwise branch need to be modified: | ||
# 1. If it can be provided by the command-line, fail and ask the user to provide it. | ||
# 2. If it cannot be provided by the command-line, prompt for it if we are running interactively. | ||
# 3. If we cannot prompt because we are not running interactively, use the default if there is one. | ||
# 4. Fail: the only way to provide a value is via the config.yml, which can be set via 'install-transpile'. | ||
|
||
if option.is_optional(): | ||
return None | ||
return option.prompt_for_value(self._prompts) | ||
|
||
def check(self) -> tuple[TranspileConfig, TranspileEngine]: | ||
"""Checks that all configuration parameters are present and valid.""" | ||
logger.debug(f"Checking config: {self._config!r}") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,16 +123,30 @@ def parse(cls, data: JsonValue) -> "LSPConfigOptionV1": | |
|
||
return LSPConfigOptionV1(flag, method, prompt, **optional) | ||
|
||
def is_optional(self) -> bool: | ||
# Semantics are currently that a value for an option is always required, except in the specific case of: | ||
# - It being a QUESTION; AND | ||
# - The default is set to the special "<none>" value. | ||
return self.method == LSPPromptMethod.QUESTION and self.default == self._question_optional_sentinel | ||
|
||
# Magic value that indicates no answer is required for a QUESTION prompt. | ||
_question_optional_sentinel = "<none>" | ||
|
||
def prompt_for_value(self, prompts: Prompts) -> JsonValue: | ||
if self.method == LSPPromptMethod.FORCE: | ||
return self.default | ||
assert self.prompt is not None | ||
if self.method == LSPPromptMethod.CONFIRM: | ||
return prompts.confirm(self.prompt) | ||
if self.method == LSPPromptMethod.QUESTION: | ||
default = self.default if self.default else "None" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sundarshankar89: This is the bit where I think it would insert the string |
||
# Hack to: | ||
# - trick prompts.question() into indicating that no answer is required; | ||
# - allow no answer to be given. | ||
# Normally prompts.confirm() requires an answer, or returns the default, and the default can't be None. | ||
# Note: LSP servers use '<none>' as a default to indicate that no answer is required. | ||
default = self.default if self.default else self._question_optional_sentinel | ||
result = prompts.question(self.prompt, default=default) | ||
if result == "<none>": | ||
if result == self._question_optional_sentinel: | ||
return None | ||
return result | ||
if self.method == LSPPromptMethod.CHOICE: | ||
|
Uh oh!
There was an error while loading. Please reload this page.