-
Notifications
You must be signed in to change notification settings - Fork 119
Refactor apps-mcp to use CLI-based approach #4003
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
378d5f3 to
70ee03b
Compare
|
Commit: 7bd295a
19 failing tests:
Top 28 slowest tests (at least 2 minutes):
|
| resp, err := w.StatementExecution.ExecuteAndWait(ctx, dbsql.ExecuteStatementRequest{ | ||
| WarehouseId: warehouseID, | ||
| Statement: statement, | ||
| WaitTimeout: "50s", |
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.
What happens on timeout? Since you're wrapping the statement, the AI might not be able to poll for progress?
|
|
||
| func readClaudeMd(ctx context.Context, configFile string) { | ||
| showFallback := func() { | ||
| cmdio.LogString(ctx, "\nConsult with CLAUDE.md provided in the bundle if present.") |
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.
Note that AGENTS.md is supported more broadly by agents, including those from Cursor, so ideally we support that as well.
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 will keep a link in the future. Claude does not read AGENTS.md by default (at least it did not).
| Examples: | ||
| experimental apps-mcp tools init-template # Choose from built-in templates | ||
| experimental apps-mcp tools init-template default-python # Python jobs and notebooks |
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'm not sure I understand why we need this command? Is it adding things that bundle init wasn't doing?
And for AI agents i think we should just always use template-minimal and then add resource-specific things like an app template on top. Ideally those resource-specific additions would be done by the bundle generate command (but we don't have any of that yet so it's good to just start with internal tool commands).
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.
This command mimics the bundle init, but looks for claude.md and on success injects its contents in the output such that the agent definitely gets the instructions. It does a couple more things
- Lets the agent discover the schema of the template to see which parameters it needs
- Allows to pass those parameters as a flag without locking into interactive prompts.
| 1 - Validation failed | ||
| 2 - Invalid flags or configuration`, | ||
| Example: ` databricks experimental apps-mcp tools validate ./my-project | ||
| databricks experimental apps-mcp tools validate ./my-project --validator=custom --custom-command="./validate.sh"`, |
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.
Is --custom-command this actually used? If not maybe we should be agile here?
Some thoughts on how I think a validation tool call might work in the future
- It could be ideal to have a single "validate" that runs all checks regardless of stack like nodejs
- We'd probably want to istinguish between all
fastchecks and allthorough/slow checks. I could imagine that all thefastchecks run all the local tests/lints/etc. Andthoroughmight run slow-running things that depend on compute like the Pipeline "dry run." - Any custom scripts might factor into this via the
scriptssection in databricks.yml. Avalidateorvalidate_fastscript could be executed as part of afastcheck. - Maybe we need an option to select a resource to check (like which pipeline, which app, which endpoint). Rather than a check? If we need that option at all.
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.
Right, it's not used now - serving as a placeholder for future extensions. Ideally we should resolve what validate should do based on the contents of the project folder and have very few flags here.
| @@ -1,18 +1,13 @@ | |||
| Your session in Databricks MCP has been successfully initialized. Here are the guidelines to follow while working on projects using databricks_mcp tools: | |||
| Your session in Databricks MCP has been successfully initialized. | |||
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.
When would this show exactly? It might not still show after context compaction/clearing?
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.
This one is shown on the first tool call regardless of was it explore or invoke_databricks_cli. Compaction and clearing do not reset us on the MCP side since we can't capture those calls from claude / cursor.
lennartkats-db
left a comment
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.
Good unification/refactoring work, @igrekun! I left some inline comments
## Changes <!-- Brief summary of your changes that is easy to understand --> ## Why <!-- Why are these changes needed? Provide the context that the reviewer might be missing. For example, were there any decisions behind the change that are not reflected in the code itself? --> ## Tests <!-- How have you tested the changes? --> <!-- If your PR needs to be included in the release notes for next release, add a separate entry in NEXT_CHANGELOG.md as part of your PR. -->
| **Initialize an app from template:** | ||
| ``` | ||
| Add a new API endpoint to fetch customer details | ||
| Initialize a new Streamlit app using the Databricks bundle template |
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.
remove reference to streamlit
| - Gives workflow guidance for Databricks Asset Bundles and Apps | ||
|
|
||
| *This is the foundation of your application - a working, tested template ready for customization.* | ||
| - **`invoke_databricks_cli`** - Execute any Databricks CLI command |
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 turned Args into an array of strings because I didn't want to execute it through a shell. We should reflect this here as well
|
|
||
| *This is the foundation of your application - a working, tested template ready for customization.* | ||
| - **`invoke_databricks_cli`** - Execute any Databricks CLI command | ||
| - Run bundle commands: `bundle init`, `bundle validate`, `bundle deploy`, `bundle run` |
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.
| - Run bundle commands: `bundle init`, `bundle validate`, `bundle deploy`, `bundle run` | |
| - Run bundle commands: `["bundle", "init"]`, `["bundle", "validate"]`, `["bundle", "deploy"]`, `["bundle", "run"]` |
|
Commit: 5759c24
24 failing tests:
Top 50 slowest tests (at least 2 minutes):
|
This refactors the apps-mcp server to use CLI commands instead of direct API providers, significantly simplifying the architecture and leveraging existing bundle command functionality.
Changes
New CLI-based provider:
Removed providers and templates:
Clean up old development features:
Why
This change reduces code complexity while providing a more maintainable architecture that reuses existing CLI commands rather than duplicating API logic.
Tests