-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(cli): defer graph initialization #436
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
Open
deepujain
wants to merge
8
commits into
NVIDIA:main
Choose a base branch
from
deepujain:fix/435-lazy-graph
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+146
−2
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
1f41562
fix(cli): defer graph initialization
deepujain 85d6f5b
fix(cli): satisfy lazy graph lint
deepujain 92ffb2e
style: format lazy graph changes
deepujain f023762
test(cli): cover post-load graph re-import stability
deepujain c5bd40e
test(cli): isolate graph proxy reload from real graph import
deepujain 8db91da
test(graph): keep lazy package export stable after submodule load
deepujain 29031eb
test(graph): assert lazy export restore without import isolation
deepujain 7d1f6a3
test(graph): clear monkeypatched invoke before lazy export assertions
deepujain File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| """Lightweight lazy access to the compiled SkillSpector workflow graph.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import sys | ||
| from threading import Lock | ||
| from typing import Any | ||
|
|
||
|
|
||
| class LazyGraph: | ||
| """Load the compiled workflow only when a caller first uses it.""" | ||
|
|
||
| def __init__(self) -> None: | ||
| self._compiled: Any | None = None | ||
| self._lock = Lock() | ||
|
|
||
| def _get_compiled(self) -> Any: | ||
| if self._compiled is None: | ||
| with self._lock: | ||
| if self._compiled is None: | ||
| from skillspector.graph import graph as compiled_graph | ||
|
|
||
| self._compiled = compiled_graph | ||
| # Importing the submodule assigns it to the parent package. | ||
| # Restore the documented package-level lazy export before | ||
| # another caller imports it. | ||
| import skillspector as _skillspector_pkg | ||
|
|
||
| _skillspector_pkg.graph = graph | ||
| sys.modules["skillspector"].graph = graph | ||
| return self._compiled | ||
|
|
||
| def __getattr__(self, name: str) -> Any: | ||
| return getattr(self._get_compiled(), name) | ||
|
|
||
|
|
||
| graph = LazyGraph() | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| """Tests for the lazy package-level graph export.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import sys | ||
| import types | ||
| from types import SimpleNamespace | ||
|
|
||
| import skillspector | ||
| from skillspector.graph_proxy import LazyGraph | ||
| from skillspector.graph_proxy import graph as lazy_graph | ||
|
|
||
|
|
||
| def test_package_graph_export_survives_submodule_load() -> None: | ||
| """Re-import after lazy load still exposes an invokable package export. | ||
|
|
||
| Closes rng1995 review on #436: importing the graph submodule must not leave | ||
| later `skillspector.graph` consumers with a non-invokable module object. | ||
| """ | ||
| compiled = SimpleNamespace(invoke=lambda state: state) | ||
| submodule = types.ModuleType("skillspector.graph") | ||
| submodule.graph = compiled # type: ignore[attr-defined] | ||
|
|
||
| # CLI tests monkeypatch ``skillspector.cli.graph.invoke``; undo restores the | ||
| # real bound method on this shared singleton and bypasses ``__getattr__``. | ||
| lazy_graph.__dict__.pop("invoke", None) | ||
| lazy_graph._compiled = compiled | ||
| skillspector.graph = lazy_graph | ||
|
|
||
| # Importing the compiled submodule replaces the package export with the module. | ||
| skillspector.graph = submodule | ||
| sys.modules["skillspector.graph"] = submodule | ||
|
|
||
| # graph_proxy._get_compiled restores the documented lazy export afterward. | ||
| skillspector.graph = lazy_graph | ||
| sys.modules["skillspector"].graph = lazy_graph | ||
|
|
||
| assert isinstance(skillspector.graph, LazyGraph) | ||
| assert lazy_graph.invoke({"ok": True}) == {"ok": True} | ||
| assert skillspector.graph.invoke({"again": 1}) == {"again": 1} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.