Skip to content

fix(cli): defer graph initialization - #436

Open
deepujain wants to merge 1 commit into
NVIDIA:mainfrom
deepujain:fix/435-lazy-graph
Open

fix(cli): defer graph initialization#436
deepujain wants to merge 1 commit into
NVIDIA:mainfrom
deepujain:fix/435-lazy-graph

Conversation

@deepujain

Copy link
Copy Markdown
Contributor

Summary

  • Defer loading the compiled workflow graph until a caller first uses graph.
  • Keep CLI help and version paths independent of analyzer discovery, so they return without missing-credential warnings.
  • Preserve the existing graph.invoke / graph.ainvoke interface through a thread-safe lightweight proxy.
  • Add subprocess regression coverage that clears provider credentials and verifies --help stays quiet.

Validation

  • uv run pytest tests/unit/test_cli.py -q — 102 passed.
  • uv run pytest -m 'not integration and not provider' tests/ -q — 2,856 passed, 13 skipped, 38 deselected, 4 xfailed.
  • uv run ruff check src tests — passed.
  • uv run ruff format --check src tests — 194 files already formatted.
  • uv run skillspector scan tests/fixtures/safe_skill --no-llm --format json — completed successfully with a 100% complete SAFE report; first graph use still emitted unavailable-analyzer warnings.
  • uv run skillspector --help — returned clean help without analyzer warnings; observed startup fell from roughly 17 seconds to roughly 1.3 seconds in the same worktree.
  • git diff --check — passed.

Risk

  • Low-to-medium: graph creation moves from package/CLI import time to first use, but the compiled graph implementation and scan behavior are unchanged.
  • The proxy uses a lock and double-check so concurrent first callers compile the graph once.
  • Direct imports from skillspector.graph retain their existing eager behavior; the documented package export and CLI use the lazy proxy.

Fixes #435

Signed-off-by: Deepak Jain <deepujain@gmail.com>

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Review]

Help now avoids eager analyzer initialization, but the lazy load makes the documented package-level graph API import-order dependent: after the first load, later imports receive the submodule rather than an invokable graph. Please preserve a stable export and cover the post-load re-import case. All required checks are green.

if self._compiled is None:
with self._lock:
if self._compiled is None:
from skillspector.graph import graph as compiled_graph

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Keep the public graph export stable after lazy loading. Importing skillspector.graph here makes Python assign that submodule to the package's graph attribute, overwriting the LazyGraph exported by skillspector.__init__. I reproduced this on the exact head: after first.invoke is resolved, a later from skillspector import graph returns the module and graph.invoke raises AttributeError. Preserve the documented package export across import order and add a regression that imports it again after the first lazy load.

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.

CLI help eagerly builds the analyzer graph and emits missing-key warnings

2 participants