Fix type hints on LSP configuration classes and improve validation when loading #2041

asnare · 2025-09-23T15:10:32Z

Changes

This PR updates type hints on the configuration classes, and implements proper checking of the configuration during loading. Changes include:

We now distinguish between missing attributes and attributes that are present but malformed.
With the options for a dialect, the prompt is now optional instead of stubbing with an empty string. (The prompt is not needed for the FIXED option type.)
For options, the combinations of choices/default are now dealt with properly depending on the kind of option it is.
Some tests that were using invalid fixtures have been fixed.

Caveats/things to watch out for when reviewing

During deserialisation, in many places we now handle unhappy paths via exceptions rather than checking in advance: the latter is unambiguous whereas its easy to have subtle issues when the check doesn't quite correctly anticipate or correspond to the actual usage.

Relevant implementation details

Sadly, most of this has been done by hand even though there are libraries that do this sort of thing for us.

Tests

existing unit tests

…from YAML. Error messages are now more accurate when something is wrong. Further to the above, some test fixtures were using invalid configurations, and this needed fixing.

github-actions · 2025-09-23T15:21:58Z

✅ 29/29 passed, 2 flaky, 1m39s total

Flaky tests:

🤪 test_transpiles_informatica_with_sparksql (9.94s)
🤪 test_transpile_sql_file (9.592s)

_{Running from acceptance #2383}

m-abulazm

this greatly improves the user experience. thanks for this

but I would have not implemented it this way. if you are working on this part, why didnt you try to use cattrs or something similar?

I also added a few suggestions to improve readability.

src/databricks/labs/lakebridge/config.py

asnare · 2025-09-24T09:22:08Z

why didnt you try to use cattrs or something similar?

That's a good question, and we've asked ourselves the same question especially with respect to similar code in blueprint. I think there's a preference for Pydantic over cattrs, but the point stands.

The general situation with respect to dependencies is:

Any new dependencies need to go through legal review. (In this case it would probably be fine, and from a practical viewpoint I think it's a transitive dependency anyway. But still legal need to look at it.)
We need to consider the situation with respect to other dependencies that use the library and how likely conflicts are. This tends to be a bit messy, and cause headaches.
It needs to be a library that we think that all developers can use and understand.
Needs to be a strong benefit to including it.

Back to the situation at hand: I agree we should be using a library to do this, and have created #2050 to cover this.

This hints at being more than just a getter.

m-abulazm

added to some of the open points and resolved others

m-abulazm

looks better. it is a good basis if more config is added in the future.
I left one more comment regarding one more refactoring.
I also don't mind leaving things as is as long we are aligned

…string_field().

…e installed transpilers (#2051) ## Changes ### What does this PR do? This PR implements a `describe-transpile` subcommand that describes the currently installed transpilers and associated dialects and configuration. This is intended for diagnostics and use by the UI. When run normally, it provides output like this: ``` % databricks labs lakebridge describe-transpile Transpiler Installed Version Plugin Configuration ========== ================= ==================== Morpheus 0.6.6 /Users/me/.databricks/labs/remorph-transpilers/databricks-morph-plugin/lib/config.yml Bladebridge 0.1.15 /Users/me/.databricks/labs/remorph-transpilers/bladebridge/lib/config.yml Supported Source Dialects ========================= - datastage - informatica (desktop edition) - informatica cloud - mssql - netezza - oracle - snowflake - synapse - teradata - tsql ``` When the `--output=json` option is provided to the Databricks CLI, more information is available: ```json % databricks labs lakebridge describe-transpile --output=json { "available-dialects": [ "datastage", "informatica (desktop edition)", "informatica cloud", "mssql", "netezza", "oracle", "snowflake", "synapse", "teradata", "tsql" ], "installed-transpilers": [ { "config-path":"/Users/andrew.snare/.databricks/labs/remorph-transpilers/databricks-morph-plugin/lib/config.yml", "name":"Morpheus", "supported-dialects": { "snowflake": { "options": [] }, "tsql": { "options": [] } }, "versions": { "installed":"0.6.6", "latest":null } }, { "config-path":"/Users/andrew.snare/.databricks/labs/remorph-transpilers/bladebridge/lib/config.yml", "name":"Bladebridge", "supported-dialects": { "datastage": { "options": [ { "default":"\u003cnone\u003e", "flag":"overrides-file", "method":"QUESTION", "prompt":"Specify the config file to override the default[Bladebridge] config - press \u003center\u003e for none" }, { "choices": [ "SPARKSQL", "PYSPARK" ], "flag":"target-tech", "method":"CHOICE", "prompt":"Specify which technology should be generated" } ] }, "informatica (desktop edition)": { "options": [ { "default":"\u003cnone\u003e", "flag":"overrides-file", "method":"QUESTION", "prompt":"Specify the config file to override the default[Bladebridge] config - press \u003center\u003e for none" }, { "choices": [ "SPARKSQL", "PYSPARK" ], "flag":"target-tech", "method":"CHOICE", "prompt":"Specify which technology should be generated" } ] }, "informatica cloud": { "options": [ { "default":"\u003cnone\u003e", "flag":"overrides-file", "method":"QUESTION", "prompt":"Specify the config file to override the default[Bladebridge] config - press \u003center\u003e for none" } ] }, "mssql": { "options": [ { "default":"\u003cnone\u003e", "flag":"overrides-file", "method":"QUESTION", "prompt":"Specify the config file to override the default[Bladebridge] config - press \u003center\u003e for none" } ] }, "netezza": { "options": [ { "default":"\u003cnone\u003e", "flag":"overrides-file", "method":"QUESTION", "prompt":"Specify the config file to override the default[Bladebridge] config - press \u003center\u003e for none" } ] }, "oracle": { "options": [ { "default":"\u003cnone\u003e", "flag":"overrides-file", "method":"QUESTION", "prompt":"Specify the config file to override the default[Bladebridge] config - press \u003center\u003e for none" } ] }, "synapse": { "options": [ { "default":"\u003cnone\u003e", "flag":"overrides-file", "method":"QUESTION", "prompt":"Specify the config file to override the default[Bladebridge] config - press \u003center\u003e for none" } ] }, "teradata": { "options": [ { "default":"\u003cnone\u003e", "flag":"overrides-file", "method":"QUESTION", "prompt":"Specify the config file to override the default[Bladebridge] config - press \u003center\u003e for none" } ] } }, "versions": { "installed":"0.1.15", "latest":null } } ] } ``` ### Relevant implementation details The formatting of the details is handled by a new `TranspilerDescription` class, so that the output format can be properly controlled. (For compatibility this will need to be controlled tightly, even if the internal details change.) Currently there is no lookup to figure out the latest version of an installed transpiler, that's out of scope for this PR although it has a position in the JSON output. ### Linked issues Additional tests in the areas modified by this code are implemented in: - #2041 - #2042 ### Functionality - added new CLI command: `databricks labs lakebridge describe-transpile` ### Tests - manually tested - added unit tests - added integration tests

m-abulazm

approved

sundarshankar89

LGTM

Type hint the LSP configuration classes and check types when reading …

d743fa5

…from YAML. Error messages are now more accurate when something is wrong. Further to the above, some test fixtures were using invalid configurations, and this needed fixing.

asnare self-assigned this Sep 23, 2025

asnare requested a review from a team as a code owner September 23, 2025 15:10

asnare added tech debt design flaws and other cascading effects internal technical pr's not end user facing labels Sep 23, 2025

asnare temporarily deployed to tool September 23, 2025 15:10 — with GitHub Actions Inactive

asnare requested review from gueniai, sundarshankar89 and m-abulazm September 23, 2025 15:18

Merge branch 'main' into lsp-config-type-checking

81dd669

asnare temporarily deployed to tool September 23, 2025 16:48 — with GitHub Actions Inactive

m-abulazm requested changes Sep 24, 2025

View reviewed changes

asnare added 2 commits September 24, 2025 10:52

Refactor pattern to limit scope of KeyError.

ca51a50

Simplify field extraction for optional field.

e341d8b

asnare temporarily deployed to tool September 24, 2025 08:56 — with GitHub Actions Inactive

Naming tweak: get -> extract

cc27347

This hints at being more than just a getter.

asnare temporarily deployed to tool September 24, 2025 09:51 — with GitHub Actions Inactive

asnare requested a review from m-abulazm September 24, 2025 09:53

asnare mentioned this pull request Sep 24, 2025

Implement a new describe-transpile CLI subcommand that describes the installed transpilers #2051

Merged

m-abulazm requested changes Sep 24, 2025

View reviewed changes

Merge branch 'main' into lsp-config-type-checking

32f8f15

asnare temporarily deployed to tool September 25, 2025 08:00 — with GitHub Actions Inactive

Extract some common field extraction code.

80a997a

asnare temporarily deployed to tool September 25, 2025 08:16 — with GitHub Actions Inactive

Clarify why the two fields can't be combined.

3165276

asnare temporarily deployed to tool September 25, 2025 08:29 — with GitHub Actions Inactive

asnare requested a review from m-abulazm September 25, 2025 08:32

m-abulazm reviewed Sep 25, 2025

View reviewed changes

asnare added 2 commits September 25, 2025 13:55

Tidy up grammar.

1490656

Update extract_string_field() to be implemented in terms of _extract_…

9a3924f

…string_field().

asnare temporarily deployed to tool September 25, 2025 11:56 — with GitHub Actions Inactive

asnare requested a review from m-abulazm September 25, 2025 12:05

Merge branch 'main' into lsp-config-type-checking

9befe55

asnare had a problem deploying to tool September 25, 2025 12:26 — with GitHub Actions Failure

Fix integration tests.

daec35d

asnare temporarily deployed to tool September 25, 2025 12:48 — with GitHub Actions Inactive

m-abulazm approved these changes Sep 25, 2025

View reviewed changes

Merge branch 'main' into lsp-config-type-checking

4c2308d

asnare temporarily deployed to tool September 25, 2025 15:30 — with GitHub Actions Inactive

Merge branch 'main' into lsp-config-type-checking

6708440

asnare temporarily deployed to tool September 26, 2025 09:33 — with GitHub Actions Inactive

sundarshankar89 approved these changes Sep 26, 2025

View reviewed changes

asnare added this pull request to the merge queue Sep 26, 2025

Merged via the queue into main with commit 7efd6db Sep 26, 2025
9 checks passed

asnare deleted the lsp-config-type-checking branch September 26, 2025 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix type hints on LSP configuration classes and improve validation when loading #2041

Fix type hints on LSP configuration classes and improve validation when loading #2041

asnare commented Sep 23, 2025

Uh oh!

github-actions bot commented Sep 23, 2025 •

edited

Loading

Uh oh!

m-abulazm left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asnare commented Sep 24, 2025

Uh oh!

m-abulazm left a comment

Uh oh!

m-abulazm left a comment

Uh oh!

m-abulazm left a comment

Uh oh!

sundarshankar89 left a comment

Uh oh!

Uh oh!

Uh oh!

Fix type hints on LSP configuration classes and improve validation when loading #2041

Fix type hints on LSP configuration classes and improve validation when loading #2041

Conversation

asnare commented Sep 23, 2025

Changes

Caveats/things to watch out for when reviewing

Relevant implementation details

Tests

Uh oh!

github-actions bot commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m-abulazm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asnare commented Sep 24, 2025

Uh oh!

m-abulazm left a comment

Choose a reason for hiding this comment

Uh oh!

m-abulazm left a comment

Choose a reason for hiding this comment

Uh oh!

m-abulazm left a comment

Choose a reason for hiding this comment

Uh oh!

sundarshankar89 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Sep 23, 2025 •

edited

Loading