Skip to content

Extract walk_catalog_schemas scaffold in databricks.py - #574

Merged
xsh310 merged 3 commits into
skills-picker-ui-packagefrom
skills-picker-walk
Sep 16, 2026
Merged

xsh310 merged 3 commits into
skills-picker-ui-packagefrom
skills-picker-walk

Conversation

@xsh310

@xsh310 xsh310 commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

What changed, and why?

Change: Extract the catalogs → schemas → parallel-per-schema-probe walk out of list_all_mcp_services into a generic walk_catalog_schemas[T](*, deadline, probe, collect, skip_catalogs=…) in databricks.py, and rewrite list_all_mcp_services on top of it. The scaffold owns catalog/schema enumeration, the worker pools, and the wall-clock deadline drain; the caller supplies the probe (list_mcp_services) and a collect that owns accumulation, dedup, progress, and streaming.

Why: Pure refactor (PR B in the ug skill add interactive-picker stack). Extracting the walk lets PR D's skills discovery reuse it as a second caller. list_all_mcp_services's signature and every return value are unchanged.

Note: the design doc's PR B also mentioned rewriting list_uc_functions_catalog_schemas, but that function does not exist in this repo — there is a single walk today, so the scaffold has one caller now and the skills walk becomes the second in PR D.

How do you know it works?

Testing: ruff check and ruff format clean; full uv run pytest green except the two pre-existing e2e failures (test_e2e_user_agent, test_claude_smart_routing_v2) that also fail on main. Added TestWalkCatalogSchemas (skips skip-catalogs + information_schema, probes each user schema, reports progress, returns the right phase-1 reason); the existing TestListAllMcpServices tests stay green unchanged.

This pull request and its description were written by Isaac.

Comment thread src/ucode/databricks.py Outdated
Comment thread src/ucode/databricks.py Outdated
Comment thread src/ucode/databricks.py Outdated
@xsh310
xsh310 force-pushed the skills-picker-walk branch 2 times, most recently from 8dfc114 to de4ed86 Compare September 15, 2026 14:43
xsh310 and others added 3 commits September 16, 2026 00:46
Factor the catalogs -> schemas -> parallel-per-schema-probe walk out of list_all_mcp_services into a generic walk_catalog_schemas(probe, collect), and rewrite list_all_mcp_services on top of it. The scaffold owns catalog/schema enumeration, worker pools, and the wall-clock deadline drain; the caller owns the probe, accumulation, dedup, progress, and streaming. No behavior change; the upcoming skills walk (download picker) becomes the second caller.

Co-authored-by: Isaac <no-reply@databricks.com>
Rewrite the docstring around the high-level behavior and spell out the probe/collect callable shapes; revert the incidental schema_name -> name rename.

Co-authored-by: Isaac <no-reply@databricks.com>
Rename _UC_FUNCTION_PROBE_WORKERS -> _SCHEMA_PROBE_WORKERS (a generic per-schema concurrency cap, not UC-function-specific) and expose it as the max_workers param, mirroring skip_catalogs.

Co-authored-by: Isaac <no-reply@databricks.com>
@xsh310
xsh310 merged commit a03df6d into main Sep 16, 2026
18 checks passed
@xsh310
xsh310 deleted the skills-picker-walk branch September 16, 2026 01:00
xsh310 added a commit that referenced this pull request Sep 16, 2026
## What

PR C of the `ug skill add` interactive-picker stack. It reshapes
`skills_download.py` so the download mechanics become one reusable core,
and adds the (still unwired) functions the picker will call in PR D.
**No behavior changes on any existing path.**

Design doc: `ug_skill_add_download_interactive_picker_design.md` (PR
table, row C).

## Changes

- **`SkillRef` carries its full location.** New `catalog`/`schema`
fields plus an `fqn` property, parsed once from the API `name` in
`_skill_ref`, so a ref is self-contained and can be downloaded on its
own.
- **Extract `_download_refs(refs, roots) -> (written, total)`**, the
shared core: collision dedup, overwrite prompt, parallel fetch, write.
`should_download_skill` and `_reject_bundle_name_collisions` now read
the source off the ref (`ref.fqn`) instead of an explicit `location`
arg, which produces byte-identical messages and lets the core serve a
flat, cross-schema set.
- **`_fetch_bundles` keys results by FQN** (a securable name is unique
only within its schema) and takes its progress-bar label from the
caller.
- **Rename `download_skills` to
`download_skills_from_schema_locations`**, now calling `_download_refs`
per location and keeping the exact per-location `Downloaded k/n from
<location>` summary.
- **Add `get_skill(fqn)`** (GetSkill at
`/2.1/unity-catalog/skills/<fqn>`, reusing `_skill_ref`; NOT_FOUND to
`None`) and **`download_selected_skills(fqns, path)`** (one
`_download_refs` pass over the flat selection). Both are dead code until
PR D wires the picker.

## Deviations from the design doc

- `download_managed_skills_on_launch` is a **second caller of
`_fetch_bundles`** the doc did not mention; it is updated to the new
keying (behavior-preserving).
- The doc lists `should_download_skill` /
`_reject_bundle_name_collisions` as reused unchanged, but making the
core location-agnostic means they read the source off the ref. The
emitted messages are identical (`<catalog>.<schema>.<securable>` equals
`ref.fqn`).

## Testing

`uv run ruff check .`, `ruff format --check src tests`, and `uv run
pytest` all pass (the 2 pre-existing e2e failures in
`test_e2e_user_agent` and `test_claude_smart_routing_v2` are unrelated
and also fail on the base branch). New unit tests cover `_download_refs`
(including cross-schema dedup), `get_skill` (NOT_FOUND to `None`,
unfinalized to `None`), and `download_selected_skills`.

Stacked on #574 (`skills-picker-walk`).

This pull request and its description were written by Isaac.

---------

Co-authored-by: Isaac <no-reply@databricks.com>
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.

2 participants