perf: defer boto3 + submodule loads to cut cold-start init#24
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughConverts nui_shared_utils to a centralized, per-attribute lazy-export dispatcher and makes nui_lambda_shared_utils a lazy forwarding shim to nui_shared_utils; defers boto3 and Slack imports to call sites; adds AWS-region/config helpers, client-error decorator, lazy CloudWatch client property, import-benchmark script, and expanded lazy/import and utils tests. (≤50 words) ChangesSingle coherent lazy-loading DAG
sequenceDiagram
participant App as Application Code
participant Init as nui_shared_utils.__init__
participant Cache as Package Globals
participant Submod as Submodule
App->>Init: import package / access symbol
Init->>Cache: check globals for cached symbol
Cache-->>Init: miss
Init->>Init: __getattr__ -> consult _LAZY_EXPORTS
Init->>Submod: importlib.import_module(submodule)
Submod-->>Init: module loaded or ImportError
Init->>Init: getattr(module, attr) or None
Init->>Cache: store resolved symbol in globals()
Init-->>App: return symbol (or None / raise)
App->>Init: subsequent access
Init->>Cache: hit -> return symbol immediately
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
nui_lambda_shared_utils/__init__.py (1)
34-43: ⚡ Quick winAdd return type hints to shim dunder APIs.
__getattr__and__dir__are public module hooks and should have explicit return annotations for MyPy/static clarity.Suggested fix
+from typing import Any + -def __getattr__(name: str): +def __getattr__(name: str) -> Any: @@ -def __dir__(): +def __dir__() -> list[str]:As per coding guidelines, "Include type hints for all public APIs."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nui_lambda_shared_utils/__init__.py` around lines 34 - 43, Add explicit return type annotations to the module dunder hooks: annotate __getattr__(name: str) -> Any and __dir__() -> list[str] (or List[str]) and import Any/List from typing if not already present; update the function signatures for __getattr__ and __dir__ in nui_lambda_shared_utils.__init__ to include these return types so static type checkers (MyPy) recognize the public API types.tests/test_lazy_imports.py (1)
24-152: ⚡ Quick winAdd pytest marker(s) for this test module.
This file defines unit tests but does not mark them with
@pytest.mark.unit(or module-levelpytestmark), which breaks the test selection convention used in this repo.Suggested fix
import subprocess import sys import textwrap +import pytest + +pytestmark = pytest.mark.unitAs per coding guidelines, "Mark test functions with appropriate pytest markers (
@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.slow)".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_lazy_imports.py` around lines 24 - 152, The test module tests/test_lazy_imports.py is missing pytest markers; add a module-level pytestmark = [pytest.mark.unit] (or apply `@pytest.mark.unit` to each test function like test_bare_package_import_does_not_load_boto3, test_jwt_auth_import_does_not_load_boto3, etc.) so test selection follows the repo convention; import pytest at the top and ensure pytestmark is defined before the tests (or annotate each test) to mark the file as unit tests.nui_shared_utils/cloudwatch_metrics.py (1)
50-57: ⚡ Quick winType-annotate the new public
clientproperty.
clientis a new public API surface and currently has no return annotation, which undercuts static checking.Suggested fix
- `@property` - def client(self): + `@property` + def client(self) -> object: """Lazily construct the boto3 CloudWatch client on first use."""As per coding guidelines, "Include type hints for all public APIs."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nui_shared_utils/cloudwatch_metrics.py` around lines 50 - 57, Add a return type for the new public property `client`: change the signature of the property method `def client(self)` to include an explicit return annotation (for example `-> BaseClient`) and import the type from `botocore.client` (or use `typing.Any` if you prefer a looser type). Keep the lazy construction logic the same; ensure the module imports `from botocore.client import BaseClient` (or `from typing import Any`) and update the property signature to `def client(self) -> BaseClient:` so static checkers recognize the public API type.nui_shared_utils/utils.py (1)
149-169: ⚡ Quick winPreserve the wrapped callable's type in
handle_client_errors.This decorator is part of the shared public surface, but the untyped
decorator/wrappersignatures make MyPy treat decorated call sites much more loosely. Please type it withCallableplusParamSpec/TypeVar(or the project's supported equivalent) so consumers keep their original parameter and return types after decoration.As per coding guidelines, "Include type hints for all public APIs" and "Use MyPy for static type checking with configuration from mypy.ini".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nui_shared_utils/utils.py` around lines 149 - 169, The decorator currently uses untyped inner functions; add typing so decorated call sites preserve their original signatures: declare P = ParamSpec("P") and R = TypeVar("R") and annotate the inner decorator to accept func: Callable[P, R] -> Callable[P, R], annotate wrapper as def wrapper(*args: P.args, **kwargs: P.kwargs) -> R, and annotate the outer decorator return type as Callable[[Callable[P, R]], Callable[P, R]] (or cast the wrapper to Callable[P, R] on return). Update imports to include ParamSpec, TypeVar, Callable (and cast if used) and apply these types to the symbols decorator and wrapper inside handle_client_errors so MyPy preserves parameter and return types.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nui_shared_utils/__init__.py`:
- Around line 128-145: The import error handling for the slack_setup module is
inconsistent, as it suppresses all ImportErrors unconditionally unlike other
optional modules which check against _OPTIONAL_SUBMODULES. To fix this, add
"slack_setup" to the _OPTIONAL_SUBMODULES set if it is optional, and update the
import logic for slack_setup to catch ImportErrors conditionally—suppressing
them only if slack_setup is listed as optional; otherwise, re-raise the error.
This aligns slack_setup error handling with the existing standard for optional
imports in the code.
In `@nui_shared_utils/powertools_helpers.py`:
- Around line 45-46: In _ensure_slack_client_loaded(), when SlackClient is
already non-None and the function returns early, also set the module-level
SLACK_CLIENT_AVAILABLE flag to True so powertools_handler sees the client as
available; update the SlackClient non-None early-return path to flip
SLACK_CLIENT_AVAILABLE (and keep the existing return behavior) so the
availability flag stays in sync with SlackClient's state.
In `@nui_shared_utils/utils.py`:
- Around line 79-90: Refactor the boto3 session region resolution to avoid broad
exception swallowing: in the block around session = boto3.session.Session() and
if session.region_name, catch only specific errors (e.g., ImportError and
botocore.exceptions.NoCredentialsError) and change the log level to warning when
session-based region resolution fails; also make the final fallback region
(currently DEFAULT_AWS_REGION/"ap-southeast-2") configurable by reading an
environment variable like AWS_REGION_FALLBACK (use DEFAULT_AWS_REGION only if
that env var is unset) so the hardcoded constant is no longer the ultimate
fallback; finally add an explicit return type hint (-> Any) to the inner wrapper
function in the handle_client_errors decorator to satisfy type checking.
In `@scripts/bench_imports.py`:
- Around line 77-79: The --runs argument can be zero or negative which yields
meaningless aggregates; after parsing (around the parser.parse_args() call)
validate args.runs > 0 and fail fast if not. Use the ArgumentParser instance
(parser) to report the error (e.g., parser.error(f"--runs must be a positive
integer, got {args.runs}")) so the script exits with a clear message rather than
producing empty/zeroed benchmark output; update the check near where args is
assigned and reference args.runs.
---
Nitpick comments:
In `@nui_lambda_shared_utils/__init__.py`:
- Around line 34-43: Add explicit return type annotations to the module dunder
hooks: annotate __getattr__(name: str) -> Any and __dir__() -> list[str] (or
List[str]) and import Any/List from typing if not already present; update the
function signatures for __getattr__ and __dir__ in
nui_lambda_shared_utils.__init__ to include these return types so static type
checkers (MyPy) recognize the public API types.
In `@nui_shared_utils/cloudwatch_metrics.py`:
- Around line 50-57: Add a return type for the new public property `client`:
change the signature of the property method `def client(self)` to include an
explicit return annotation (for example `-> BaseClient`) and import the type
from `botocore.client` (or use `typing.Any` if you prefer a looser type). Keep
the lazy construction logic the same; ensure the module imports `from
botocore.client import BaseClient` (or `from typing import Any`) and update the
property signature to `def client(self) -> BaseClient:` so static checkers
recognize the public API type.
In `@nui_shared_utils/utils.py`:
- Around line 149-169: The decorator currently uses untyped inner functions; add
typing so decorated call sites preserve their original signatures: declare P =
ParamSpec("P") and R = TypeVar("R") and annotate the inner decorator to accept
func: Callable[P, R] -> Callable[P, R], annotate wrapper as def wrapper(*args:
P.args, **kwargs: P.kwargs) -> R, and annotate the outer decorator return type
as Callable[[Callable[P, R]], Callable[P, R]] (or cast the wrapper to
Callable[P, R] on return). Update imports to include ParamSpec, TypeVar,
Callable (and cast if used) and apply these types to the symbols decorator and
wrapper inside handle_client_errors so MyPy preserves parameter and return
types.
In `@tests/test_lazy_imports.py`:
- Around line 24-152: The test module tests/test_lazy_imports.py is missing
pytest markers; add a module-level pytestmark = [pytest.mark.unit] (or apply
`@pytest.mark.unit` to each test function like
test_bare_package_import_does_not_load_boto3,
test_jwt_auth_import_does_not_load_boto3, etc.) so test selection follows the
repo convention; import pytest at the top and ensure pytestmark is defined
before the tests (or annotate each test) to mark the file as unit tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ce5366f-670c-47b4-b5c4-019e5a79214e
📒 Files selected for processing (10)
nui_lambda_shared_utils/__init__.pynui_shared_utils/__init__.pynui_shared_utils/cloudwatch_metrics.pynui_shared_utils/powertools_helpers.pynui_shared_utils/secrets_helper.pynui_shared_utils/utils.pyscripts/bench_imports.pytests/conftest.pytests/test_cloudwatch_metrics.pytests/test_lazy_imports.py
hampsterx
force-pushed
the
feat/lazy-imports-cold-start
branch
from
f64a970 to
a9de761
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_lazy_imports.py (1)
24-151: ⚡ Quick winAdd explicit pytest markers for this module’s tests.
These test functions are currently unmarked. Add module-level/function-level pytest markers so selection and reporting stay consistent with repo standards.
Proposed fix
import subprocess import sys import textwrap +import pytest + +pytestmark = pytest.mark.unitAs per coding guidelines, "Mark test functions with appropriate pytest markers (
@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.slow)".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_lazy_imports.py` around lines 24 - 151, Import pytest at the top of the tests and add pytest markers so all tests in this module are explicitly marked (either add a module-level pytestmark = pytest.mark.unit or decorate individual test functions such as test_bare_package_import_does_not_load_boto3, test_jwt_auth_import_does_not_load_boto3, test_powertools_helpers_import_does_not_load_boto3, test_powertools_helpers_does_not_pull_slack_sdk, test_legacy_shim_attribute_access_is_lazy, test_secrets_helper_loads_boto3_only_on_first_call, test_lazy_attribute_access_returns_real_object, test_unknown_attribute_raises_attribute_error, test_lazy_attribute_is_cached_after_first_access, test_dir_includes_lazy_exports with `@pytest.mark.unit` or other appropriate markers like `@pytest.mark.integration/`@pytest.mark.slow where applicable); ensure the pytest import exists and the chosen marker(s) follow the repo guideline so test selection and reporting are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_lazy_imports.py`:
- Around line 100-111: Wrap the
test_secrets_helper_loads_boto3_only_on_first_call execution in moto's Secrets
Manager mock (use moto.mock_secretsmanager as a decorator or context manager) so
secrets_helper.get_secret does not hit real AWS; within the mocked context
create a dummy secret via boto3.client("secretsmanager").create_secret(...)
before calling secrets_helper.get_secret to ensure the call succeeds and boto3
is only loaded on first call. Ensure the _run snippet (which imports
secrets_helper and calls secrets_helper.get_secret) executes inside the
moto.mock_secretsmanager context so the test remains deterministic and no real
API calls occur.
- Around line 13-21: The helper _run currently calls subprocess.run without a
timeout and can hang; modify _run to accept an optional timeout (e.g.,
timeout_seconds: float = 10) and pass that into the subprocess.run call via its
timeout=... parameter (or if you prefer a fixed policy, add timeout=10 directly
to subprocess.run). Update _run's signature and any callers/tests accordingly so
the child interpreter will be killed after the timeout instead of stalling the
test suite.
---
Nitpick comments:
In `@tests/test_lazy_imports.py`:
- Around line 24-151: Import pytest at the top of the tests and add pytest
markers so all tests in this module are explicitly marked (either add a
module-level pytestmark = pytest.mark.unit or decorate individual test functions
such as test_bare_package_import_does_not_load_boto3,
test_jwt_auth_import_does_not_load_boto3,
test_powertools_helpers_import_does_not_load_boto3,
test_powertools_helpers_does_not_pull_slack_sdk,
test_legacy_shim_attribute_access_is_lazy,
test_secrets_helper_loads_boto3_only_on_first_call,
test_lazy_attribute_access_returns_real_object,
test_unknown_attribute_raises_attribute_error,
test_lazy_attribute_is_cached_after_first_access, test_dir_includes_lazy_exports
with `@pytest.mark.unit` or other appropriate markers like
`@pytest.mark.integration/`@pytest.mark.slow where applicable); ensure the pytest
import exists and the chosen marker(s) follow the repo guideline so test
selection and reporting are consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dfb91a0a-67bf-4b7f-8cb4-dde1912c1abf
📒 Files selected for processing (10)
nui_lambda_shared_utils/__init__.pynui_shared_utils/__init__.pynui_shared_utils/cloudwatch_metrics.pynui_shared_utils/powertools_helpers.pynui_shared_utils/secrets_helper.pynui_shared_utils/utils.pyscripts/bench_imports.pytests/conftest.pytests/test_cloudwatch_metrics.pytests/test_lazy_imports.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_cloudwatch_metrics.py
hampsterx
force-pushed
the
feat/lazy-imports-cold-start
branch
from
a9de761 to
7b617f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/test_lazy_imports.py (2)
13-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a subprocess timeout to prevent stalled test runs.
Line 15 executes a child interpreter with no timeout, so a blocked child can hang the suite.
Proposed fix
-def _run(script: str) -> str: +def _run(script: str, timeout_seconds: float = 20.0) -> str: """Execute ``script`` in a fresh Python interpreter and return stdout.""" result = subprocess.run( [sys.executable, "-c", textwrap.dedent(script)], capture_output=True, text=True, check=True, + timeout=timeout_seconds, ) return result.stdout.strip()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_lazy_imports.py` around lines 13 - 21, The helper _run currently calls subprocess.run without a timeout, which can hang tests; update the subprocess.run invocation inside function _run to include a sensible timeout (e.g., timeout=10) so child interpreters cannot block indefinitely, and ensure the test harness will fail fast on timeout (allowing the TimeoutExpired to propagate or be handled as desired).
100-111:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMock AWS in the secrets test to keep it deterministic.
Line 107 can attempt a real Secrets Manager call today; this should run under moto so CI does not depend on credentials/network.
Proposed fix
output = _run(""" import sys + from moto import mock_aws from nui_shared_utils import secrets_helper print(f"after_import={'boto3' in sys.modules}") - try: - secrets_helper.get_secret("nonexistent-secret-for-test") - except Exception: - pass + with mock_aws(): + try: + secrets_helper.get_secret("nonexistent-secret-for-test") + except Exception: + pass print(f"after_call={'boto3' in sys.modules}") """)As per coding guidelines, "Use moto for AWS service mocking in integration tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_lazy_imports.py` around lines 100 - 111, The test test_secrets_helper_loads_boto3_only_on_first_call may trigger a real AWS Secrets Manager request; wrap the portion that calls secrets_helper.get_secret in a moto Secrets Manager mock (use moto.mock_secretsmanager as a decorator or context manager) and create a dummy secret in the mocked service before calling get_secret so the call is deterministic and no real network/credentials are used; keep the import and the printed checks as-is so the test still verifies lazy import behavior while running entirely against moto.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nui_lambda_shared_utils/__init__.py`:
- Around line 34-43: Add explicit return type annotations to the module-level
magic functions: change the signature of __getattr__ to include -> Any (import
Any from typing if not present) and change __dir__ to include -> list[str] (or
-> List[str] with a typing import if you prefer py3.8 compatibility); update the
function definitions for __getattr__ and __dir__ accordingly and ensure
necessary typing imports (Any, List) are added to the module top.
---
Duplicate comments:
In `@tests/test_lazy_imports.py`:
- Around line 13-21: The helper _run currently calls subprocess.run without a
timeout, which can hang tests; update the subprocess.run invocation inside
function _run to include a sensible timeout (e.g., timeout=10) so child
interpreters cannot block indefinitely, and ensure the test harness will fail
fast on timeout (allowing the TimeoutExpired to propagate or be handled as
desired).
- Around line 100-111: The test
test_secrets_helper_loads_boto3_only_on_first_call may trigger a real AWS
Secrets Manager request; wrap the portion that calls secrets_helper.get_secret
in a moto Secrets Manager mock (use moto.mock_secretsmanager as a decorator or
context manager) and create a dummy secret in the mocked service before calling
get_secret so the call is deterministic and no real network/credentials are
used; keep the import and the printed checks as-is so the test still verifies
lazy import behavior while running entirely against moto.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69d24d84-e8b3-4dce-b67a-e288c9f22f26
📒 Files selected for processing (11)
nui_lambda_shared_utils/__init__.pynui_shared_utils/__init__.pynui_shared_utils/cloudwatch_metrics.pynui_shared_utils/powertools_helpers.pynui_shared_utils/secrets_helper.pynui_shared_utils/utils.pyscripts/bench_imports.pytests/conftest.pytests/test_cloudwatch_metrics.pytests/test_lazy_imports.pytests/test_utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- nui_shared_utils/secrets_helper.py
hampsterx
force-pushed
the
feat/lazy-imports-cold-start
branch
from
7b617f6 to
6e6648d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
scripts/bench_imports.py (1)
42-47: ⚡ Quick winAdd a timeout to the benchmark subprocess call.
Line 42 can block indefinitely if an import hangs, which stalls the whole benchmark run.
Suggested change
def measure(entry: str) -> dict: """Run a single ``python -X importtime`` invocation and parse the output.""" - proc = subprocess.run( - [sys.executable, "-X", "importtime", "-c", f"import {entry}"], - capture_output=True, - text=True, - check=True, - ) + try: + proc = subprocess.run( + [sys.executable, "-X", "importtime", "-c", f"import {entry}"], + capture_output=True, + text=True, + check=True, + timeout=30, + ) + except subprocess.TimeoutExpired as exc: + raise RuntimeError(f"Timed out while importing {entry!r}") from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/bench_imports.py` around lines 42 - 47, The subprocess.run call that assigns to proc in scripts/bench_imports.py can block indefinitely; add a timeout argument (e.g., timeout=10 or a configurable TIMEOUT_SECONDS) to the subprocess.run call and wrap the call in a try/except that catches subprocess.TimeoutExpired so the benchmark can log/handle a hung import (using the existing logging/print flow) and continue or record the timeout result; update references around proc, subprocess.run and handle subprocess.TimeoutExpired accordingly.nui_shared_utils/cloudwatch_metrics.py (1)
46-57: ⚡ Quick winAnnotate the new
clientproperty.
MetricsPublisher.clientis part of the public surface now, but it has no return type and_clientstays untyped. That leaves the new lazy path outside MyPy coverage.Suggested fix
-from typing import Dict, List, Optional, Union +from typing import Any, Dict, List, Optional, Union @@ - self._client = None + self._client: Optional[Any] = None @@ - def client(self): + def client(self) -> Any: """Lazily construct the boto3 CloudWatch client on first use."""As per coding guidelines, "Include type hints for all public APIs" and "Use MyPy for static type checking with configuration from mypy.ini".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nui_shared_utils/cloudwatch_metrics.py` around lines 46 - 57, Annotate the lazy CloudWatch client: add a type for the internal attribute (e.g. change self._client to self._client: Optional[BaseClient] or Optional[botocore.client.BaseClient]) and give the MetricsPublisher.client property an explicit return type (e.g. def client(self) -> BaseClient). Import the required typing names (Optional) and the BaseClient type from botocore.client (or another appropriate boto3/client base type) so MyPy will cover both the attribute and the property.tests/test_lazy_imports.py (1)
8-23: ⚡ Quick winMark this new module as unit tests.
This file is missing the repo's pytest marker, so these regression tests won't participate cleanly in marker-based selection.
Suggested fix
+import pytest import subprocess import sys import textwrap + +pytestmark = pytest.mark.unitAs per coding guidelines, "Mark test functions with appropriate pytest markers (
@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.slow)".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_lazy_imports.py` around lines 8 - 23, This module lacks the pytest unit marker: add "import pytest" and set a module-level marker (e.g., pytestmark = pytest.mark.unit) or add `@pytest.mark.unit` to the test functions in this file so the tests (including the helper _run and the tests that call it) are selected by marker-based runs; place the marker near the top of the module after imports to ensure all tests are marked.nui_shared_utils/utils.py (1)
99-134: ⚡ Quick winFinish typing the newly exported helper signatures.
create_aws_client,safe_close_connection, andformat_log_contextare public helpers, but their signatures are still partially untyped. That weakens the shared contract right where this PR expands the reusable utils surface.Suggested fix
-def create_aws_client(service_name: str, region: Optional[str] = None): +def create_aws_client(service_name: str, region: Optional[str] = None) -> Any: @@ -def safe_close_connection(connection) -> None: +def safe_close_connection(connection: Any) -> None: @@ -def format_log_context(operation: str, **context_data) -> Dict[str, Any]: +def format_log_context(operation: str, **context_data: Any) -> Dict[str, Any]:As per coding guidelines, "Include type hints for all public APIs" and "Use MyPy for static type checking with configuration from mypy.ini".
Also applies to: 228-277
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nui_shared_utils/utils.py` around lines 99 - 134, Add full type hints for the newly exported helpers: annotate create_aws_client(service_name: str, region: Optional[str] = None) to return a botocore.client.BaseClient (import BaseClient from botocore.client or use typing.Any if you prefer), annotate safe_close_connection(connection: Optional[Any]) -> None, and annotate format_log_context(context: Mapping[str, Any]) -> str (import Mapping, Any, Optional from typing); update imports accordingly and run MyPy to ensure the module passes static checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nui_shared_utils/__init__.py`:
- Around line 33-39: The lazy export table _LAZY_EXPORTS is missing several
utils symbols so callers can't import resolve_aws_region, safe_close_connection,
format_log_context, or DEFAULT_AWS_REGION from the package; update the
_LAZY_EXPORTS mapping to include these entries (pointing to the "utils" module
and the exact symbol names) in the same "Common utilities" section where
resolve_config_value/create_aws_client/etc. are declared so they are exported
via the package shim and show up in __all__/dir().
---
Nitpick comments:
In `@nui_shared_utils/cloudwatch_metrics.py`:
- Around line 46-57: Annotate the lazy CloudWatch client: add a type for the
internal attribute (e.g. change self._client to self._client:
Optional[BaseClient] or Optional[botocore.client.BaseClient]) and give the
MetricsPublisher.client property an explicit return type (e.g. def client(self)
-> BaseClient). Import the required typing names (Optional) and the BaseClient
type from botocore.client (or another appropriate boto3/client base type) so
MyPy will cover both the attribute and the property.
In `@nui_shared_utils/utils.py`:
- Around line 99-134: Add full type hints for the newly exported helpers:
annotate create_aws_client(service_name: str, region: Optional[str] = None) to
return a botocore.client.BaseClient (import BaseClient from botocore.client or
use typing.Any if you prefer), annotate safe_close_connection(connection:
Optional[Any]) -> None, and annotate format_log_context(context: Mapping[str,
Any]) -> str (import Mapping, Any, Optional from typing); update imports
accordingly and run MyPy to ensure the module passes static checks.
In `@scripts/bench_imports.py`:
- Around line 42-47: The subprocess.run call that assigns to proc in
scripts/bench_imports.py can block indefinitely; add a timeout argument (e.g.,
timeout=10 or a configurable TIMEOUT_SECONDS) to the subprocess.run call and
wrap the call in a try/except that catches subprocess.TimeoutExpired so the
benchmark can log/handle a hung import (using the existing logging/print flow)
and continue or record the timeout result; update references around proc,
subprocess.run and handle subprocess.TimeoutExpired accordingly.
In `@tests/test_lazy_imports.py`:
- Around line 8-23: This module lacks the pytest unit marker: add "import
pytest" and set a module-level marker (e.g., pytestmark = pytest.mark.unit) or
add `@pytest.mark.unit` to the test functions in this file so the tests (including
the helper _run and the tests that call it) are selected by marker-based runs;
place the marker near the top of the module after imports to ensure all tests
are marked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15c2594e-4a2a-44eb-80bc-1ab13ed6852b
📒 Files selected for processing (11)
nui_lambda_shared_utils/__init__.pynui_shared_utils/__init__.pynui_shared_utils/cloudwatch_metrics.pynui_shared_utils/powertools_helpers.pynui_shared_utils/secrets_helper.pynui_shared_utils/utils.pyscripts/bench_imports.pytests/conftest.pytests/test_cloudwatch_metrics.pytests/test_lazy_imports.pytests/test_utils.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_cloudwatch_metrics.py
- nui_shared_utils/secrets_helper.py
- nui_lambda_shared_utils/init.py
hampsterx
force-pushed
the
feat/lazy-imports-cold-start
branch
from
6e6648d to
086f9ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
nui_shared_utils/cloudwatch_metrics.py (1)
50-57: ⚡ Quick winType the new
clientproperty explicitly.
MetricsPublisher.clientis now part of the public surface, but it returns an untyped value. That weakens MyPy for every caller that touches it.Suggested typing update
-from typing import Dict, List, Optional, Union +from typing import Any, Dict, List, Optional, Union @@ - self._client = None + self._client: Any | None = None @@ - def client(self): + def client(self) -> Any:As per coding guidelines, "Include type hints for all public APIs" and "Use MyPy for static type checking with configuration from mypy.ini".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nui_shared_utils/cloudwatch_metrics.py` around lines 50 - 57, The client property on MetricsPublisher should have an explicit return type instead of untyped; annotate the property as returning botocore.client.BaseClient (or typing.Any if BaseClient isn't available), and ensure the backing attribute _client is typed as Optional[BaseClient]; import BaseClient from botocore.client (or use typing.Any) and update the MetricsPublisher class attribute/type hints so the client() method signature is like "def client(self) -> BaseClient" and _client is Optional[BaseClient], keeping the lazy initialization logic using boto3.client("cloudwatch", region_name=self._region).nui_shared_utils/utils.py (1)
99-134: ⚡ Quick winFinish typing the new utility exports.
create_aws_clientstill has no return annotation, andsafe_close_connectiontakes an untyped argument even though both helpers are now exported from the package surface. That leaves MyPy blind on two of the new APIs.Suggested typing update
-def create_aws_client(service_name: str, region: Optional[str] = None): +def create_aws_client(service_name: str, region: Optional[str] = None) -> Any: @@ -def safe_close_connection(connection) -> None: +def safe_close_connection(connection: Any | None) -> None:As per coding guidelines, "Include type hints for all public APIs" and "Use MyPy for static type checking with configuration from mypy.ini".
Also applies to: 228-250
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nui_shared_utils/utils.py` around lines 99 - 134, The public helpers lack type hints: add a return annotation to create_aws_client (use botocore.client.BaseClient as the return type or typing.Any if you prefer) and type the parameter for safe_close_connection (e.g., client: Optional[botocore.client.BaseClient]) and its return type of None; import the needed symbols (from botocore.client import BaseClient and typing imports) and update both function signatures (create_aws_client -> BaseClient, safe_close_connection(client: Optional[BaseClient]) -> None) so MyPy can validate the package surface, then run MyPy to confirm no further public-API typing issues.tests/test_lazy_imports.py (1)
25-34: 💤 Low valueConsider adding pytest markers for test categorization.
Per coding guidelines, test functions should be marked with appropriate pytest markers (
@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.slow). The subprocess-based tests (which spawn fresh interpreters) could be marked as@pytest.mark.integrationor@pytest.mark.slow, while the in-process tests (lines 125-160) are unit tests.This enables selective test execution during development and CI.
Example markers
import pytest `@pytest.mark.integration` def test_bare_package_import_does_not_load_boto3(): ... `@pytest.mark.unit` def test_lazy_attribute_access_returns_real_object(): ...As per coding guidelines: "Mark test functions with appropriate pytest markers (
@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.slow)".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_lazy_imports.py` around lines 25 - 34, Add the appropriate pytest marker to the subprocess-style test by importing pytest and decorating test_bare_package_import_does_not_load_boto3 with `@pytest.mark.integration` (or `@pytest.mark.slow` if preferred); locate the function name test_bare_package_import_does_not_load_boto3 in tests/test_lazy_imports.py, add "import pytest" at the top if missing, and apply the decorator directly above the def line so the test is categorized for selective runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nui_shared_utils/__init__.py`:
- Line 263: Ruff's PLE0605 flags the dynamic assignment to __all__ using
sorted(...); change the assignment so __all__ is a concrete list by wrapping the
sorted call with list(...) (i.e., set __all__ to list(sorted(set(_LAZY_EXPORTS)
| {"slack_setup"}))) so the symbol __all__ (and _LAZY_EXPORTS / "slack_setup")
is assigned a literal list form acceptable to the linter.
---
Nitpick comments:
In `@nui_shared_utils/cloudwatch_metrics.py`:
- Around line 50-57: The client property on MetricsPublisher should have an
explicit return type instead of untyped; annotate the property as returning
botocore.client.BaseClient (or typing.Any if BaseClient isn't available), and
ensure the backing attribute _client is typed as Optional[BaseClient]; import
BaseClient from botocore.client (or use typing.Any) and update the
MetricsPublisher class attribute/type hints so the client() method signature is
like "def client(self) -> BaseClient" and _client is Optional[BaseClient],
keeping the lazy initialization logic using boto3.client("cloudwatch",
region_name=self._region).
In `@nui_shared_utils/utils.py`:
- Around line 99-134: The public helpers lack type hints: add a return
annotation to create_aws_client (use botocore.client.BaseClient as the return
type or typing.Any if you prefer) and type the parameter for
safe_close_connection (e.g., client: Optional[botocore.client.BaseClient]) and
its return type of None; import the needed symbols (from botocore.client import
BaseClient and typing imports) and update both function signatures
(create_aws_client -> BaseClient, safe_close_connection(client:
Optional[BaseClient]) -> None) so MyPy can validate the package surface, then
run MyPy to confirm no further public-API typing issues.
In `@tests/test_lazy_imports.py`:
- Around line 25-34: Add the appropriate pytest marker to the subprocess-style
test by importing pytest and decorating
test_bare_package_import_does_not_load_boto3 with `@pytest.mark.integration` (or
`@pytest.mark.slow` if preferred); locate the function name
test_bare_package_import_does_not_load_boto3 in tests/test_lazy_imports.py, add
"import pytest" at the top if missing, and apply the decorator directly above
the def line so the test is categorized for selective runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39b3884e-80c1-48c9-bbaa-b6d29ca74d25
📒 Files selected for processing (11)
nui_lambda_shared_utils/__init__.pynui_shared_utils/__init__.pynui_shared_utils/cloudwatch_metrics.pynui_shared_utils/powertools_helpers.pynui_shared_utils/secrets_helper.pynui_shared_utils/utils.pyscripts/bench_imports.pytests/conftest.pytests/test_cloudwatch_metrics.pytests/test_lazy_imports.pytests/test_utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/conftest.py
- boto3/botocore no longer imported at module load. Deferred to first use in secrets_helper, utils, and cloudwatch_metrics (MetricsPublisher lazy-constructs its boto3 client on first publish). - Package __init__ uses PEP 562 __getattr__: submodules (slack_client, es_client, db_client, jwt_auth, powertools_helpers, etc.) only load when their attributes are first accessed. `import nui_shared_utils` drops from ~700ms to ~10ms locally. - powertools_helpers no longer eagerly imports SlackClient. slack_sdk is only pulled when slack_alert_channel is passed to powertools_handler. - Legacy nui_lambda_shared_utils shim now forwards via PEP 562 instead of `from nui_shared_utils import *`, so consumers still on the old import name also benefit from the cold-start savings. Backwards compatibility: - Public API unchanged (verified by 517-test suite + 10 new lazy tests) - Star imports still work via __all__ - Legacy nui_lambda_shared_utils import name still works - Optional integrations (SlackClient etc.) still return None when their dep is not installed
hampsterx
force-pushed
the
feat/lazy-imports-cold-start
branch
from
086f9ed to
c0a6690
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
boto3andbotocoreare no longer imported at module load. Deferred to first use insidesecrets_helper.get_secret,utils.create_aws_client/resolve_aws_region, andcloudwatch_metrics.MetricsPublisher(lazy-constructs its boto3 client on first publish).__init__uses PEP 562__getattr__: submodules (slack_client,es_client,db_client,jwt_auth,powertools_helpers, etc.) only load when their attributes are first accessed. Bareimport nui_shared_utilsdrops from ~700ms to ~10ms locally.powertools_helpersno longer eagerly importsSlackClient.slack_sdkis only pulled whenslack_alert_channelis passed topowertools_handler(~150ms desktop saving for any consumer usingget_powertools_logger).nui_lambda_shared_utilsshim now forwards via PEP 562 instead offrom nui_shared_utils import *, so consumers still on the old import name also benefit.Backwards compatibility
__all__.nui_lambda_shared_utilsimport name still works (now lazy too).SlackClient,ElasticsearchClient, etc.) still resolve toNonewhen their backing dependency is not installed. This pre-1.4 contract is preserved by design (external reviewers flagged theImportError -> Nonepattern; left intact to avoid an unrelated breaking change to the optional-extras API).Review
--jsonflag rename to--out).Test plan
pytestreports 517 passedtests/test_lazy_imports.pyassertboto3/slack_sdk/ optional submodules stay out ofsys.modulesafter lightweight imports (bare package,jwt_auth,powertools_helpers, legacy shim entry points)scripts/bench_imports.py(3-run averages):import nui_shared_utils: ~9ms (was ~700ms+)from nui_shared_utils.jwt_auth import check_auth: ~30msfrom nui_shared_utils.powertools_helpers import get_powertools_logger: ~22ms (was ~180ms with slack_sdk leak)connect-lambda-fieldsdev (10 cold-start samples): Init Duration 1481-1827ms, median 1745ms vs ~1850ms baseline. Lambda runtime pre-loadsboto3from/var/runtimeso the absolute saving is smaller than the desktop bench shows; the win is bigger for non-VPC consumers and for consumers using onlyjwt_auth/powertools_helpers.Summary by CodeRabbit
Refactor
New Features
New Tools
Tests