-
Notifications
You must be signed in to change notification settings - Fork 71
Implement a new describe-transpile
CLI subcommand that describes the installed transpilers
#2051
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
Conversation
…talled transpilers and their version.
In addition, it now sets up the repository to include version information for the transpilers.
…he installed transpilers.
✅ 29/29 passed, 2 flaky, 1m17s total Flaky tests:
Running from acceptance #2358 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
nit: I would like to see which source is supported by which transpiler
and Docs update about new comment.
The dialects for each transpiler are listed in the JSON output. A choice I made here was that the normal non-JSON output is a high-level summary, and the JSON view provides the gory details. Agreed on documentation, that's a big gap and something I need to rectify. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I just have one comment/question
all_configs = self.all_transpiler_configs() | ||
return frozenset(all_configs.keys()) | ||
|
||
def installed_transpilers(self) -> Mapping[str, TranspilerInfo]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we use this for adding the transpiler version in the telemetry? and it might make sense to cache this @cached_property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use this for obtaining the transpiler version, but it's overkill: for telemetry you only need the version that's in use, not the whole set or other meta-data.
Regarding @cached_property
I considered this but decided not to:
- It's reasonably expensive to calculate, which makes it not very property-like.
- For all our use-cases it's only called once. (This isn't a property that various components access on a repository instance that is passed around.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually consider caching if something is expensive to calculate otherwise I dont see the benefit.
I get that it is used once but since it is public, it can happen but highly unlikely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved. also responded to the previous review
all_configs = self.all_transpiler_configs() | ||
return frozenset(all_configs.keys()) | ||
|
||
def installed_transpilers(self) -> Mapping[str, TranspilerInfo]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually consider caching if something is expensive to calculate otherwise I dont see the benefit.
I get that it is used once but since it is public, it can happen but highly unlikely
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:When the
--output=json
option is provided to the Databricks CLI, more information is available: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:
TranspilerRepository
, and additional integration tests #2042Functionality
databricks labs lakebridge describe-transpile
Tests