Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion samcli/commands/local/cli_common/invoke_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ def __init__(
mount_symlinks: Optional[bool] = False,
no_mem_limit: Optional[bool] = False,
function_logical_ids: Optional[Tuple[str, ...]] = None,
no_reload: bool = False,
) -> None:
"""
Initialize the context
Expand Down Expand Up @@ -220,6 +221,7 @@ def __init__(

self._mount_symlinks: Optional[bool] = mount_symlinks
self._no_mem_limit = no_mem_limit
self._no_reload = no_reload

# Note(xinhol): despite self._function_provider and self._stacks are initialized as None
# they will be assigned with a non-None value in __enter__() and
Expand Down Expand Up @@ -256,7 +258,6 @@ def __enter__(self) -> "InvokeContext":
ContainersMode.WARM: [self._stacks, self._parameter_overrides, self._global_parameter_overrides],
ContainersMode.COLD: [self._stacks],
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Nit: Unrelated whitespace change

This removes a blank line that was separating two logical blocks. It's a minor formatting change but unrelated to the feature — I'd restore the original blank line to avoid noise in the diff.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🟡 Unrelated whitespace change: This removes a blank line that was separating the dict definitions from the following comment block. Minor, but it's unrelated to the feature and adds noise to the diff. Consider reverting.


# don't resolve the code URI immediately if we passed in docker vol by passing True for use_raw_codeuri
# this way at the end the code URI will get resolved against the basedir option
if self._docker_volume_basedir:
Expand All @@ -267,6 +268,9 @@ def __enter__(self) -> "InvokeContext":
if self._function_logical_ids:
_function_providers_kwargs["function_logical_ids"] = self._function_logical_ids

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🟡 Consider warning when --no-reload is used without --warm-containers: Currently no_reload is only passed to the function provider when containers_mode == WARM. If a user passes --no-reload without --warm-containers, it's silently ignored. A LOG.warning() (or a click validation callback) would improve the UX:

if self._no_reload and self._containers_mode != ContainersMode.WARM:
    LOG.warning("--no-reload has no effect without --warm-containers")

if self._containers_mode == ContainersMode.WARM:
_function_providers_kwargs["no_reload"] = self._no_reload

self._function_provider = _function_providers_class[self._containers_mode](
*_function_providers_args[self._containers_mode], **_function_providers_kwargs
)
Expand Down Expand Up @@ -573,6 +577,7 @@ def lambda_runtime(self) -> LambdaRuntime:
image_builder,
mount_symlinks=self._mount_symlinks,
no_mem_limit=self._no_mem_limit,
no_reload=self._no_reload,
),
ContainersMode.COLD: LambdaRuntime(
self._container_manager,
Expand Down
7 changes: 7 additions & 0 deletions samcli/commands/local/cli_common/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,13 @@ def warm_containers_common_options(f):
type=click.STRING,
multiple=False,
),
click.option(
"--no-reload",
is_flag=True,

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Consider: Should --no-reload warn or error when used without --warm-containers?

Since --no-reload is only meaningful with warm containers, using it with cold containers silently does nothing. This could be confusing to users. Consider either:

  1. Emitting a warning when --no-reload is passed without --warm-containers, or
  2. Documenting in the help text that it requires --warm-containers.

At minimum, updating the help text to mention the dependency:

"Disable automatic reloading of Lambda functions when source code or templates change. "
"Only applies when used with --warm-containers. "
"Useful on Windows where file watchers can cause significant latency with security software."

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🟢 Nice help text — clearly explains the use case. Consider also mentioning that this flag only takes effect with --warm-containers so users know upfront:

"Disable automatic reloading of Lambda functions when source code or templates change. "
"Only effective with --warm-containers. "
"Useful on Windows where file watchers can cause significant latency with security software."

default=False,
help="Disable automatic reloading of Lambda functions when source code or templates change. "
"Useful on Windows where file watchers can cause significant latency with security software.",
),
]

# Reverse the list to maintain ordering of options in help text printed with --help
Expand Down
4 changes: 4 additions & 0 deletions samcli/commands/local/start_api/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ def cli(
warm_containers,
shutdown,
debug_function,
no_reload,
container_host,
container_host_interface,
add_host,
Expand Down Expand Up @@ -174,6 +175,7 @@ def cli(
warm_containers,
shutdown,
debug_function,
no_reload,
container_host,
container_host_interface,
add_host,
Expand Down Expand Up @@ -207,6 +209,7 @@ def do_cli( # pylint: disable=R0914
warm_containers,
shutdown,
debug_function,
no_reload,
container_host,
container_host_interface,
add_host,
Expand Down Expand Up @@ -261,6 +264,7 @@ def do_cli( # pylint: disable=R0914
invoke_images=processed_invoke_images,
add_host=add_host,
no_mem_limit=no_mem_limit,
no_reload=no_reload,
) as invoke_context:
ssl_context = (ssl_cert_file, ssl_key_file) if ssl_cert_file else None
service = LocalApiService(
Expand Down
4 changes: 4 additions & 0 deletions samcli/commands/local/start_lambda/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ def cli(
warm_containers,
shutdown,
debug_function,
no_reload,
container_host,
container_host_interface,
add_host,
Expand Down Expand Up @@ -135,6 +136,7 @@ def cli(
warm_containers,
shutdown,
debug_function,
no_reload,
container_host,
container_host_interface,
add_host,
Expand Down Expand Up @@ -165,6 +167,7 @@ def do_cli( # pylint: disable=R0914
warm_containers,
shutdown,
debug_function,
no_reload,
container_host,
container_host_interface,
add_host,
Expand Down Expand Up @@ -218,6 +221,7 @@ def do_cli( # pylint: disable=R0914
invoke_images=processed_invoke_images,
function_logical_ids=function_logical_ids,
no_mem_limit=no_mem_limit,
no_reload=no_reload,
) as invoke_context:
service = LocalLambdaService(lambda_invoke_context=invoke_context, port=port, host=host)
service.start()
Expand Down
6 changes: 5 additions & 1 deletion samcli/lib/providers/sam_function_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -894,6 +894,7 @@ def __init__(
use_raw_codeuri: bool = False,
ignore_code_extraction_warnings: bool = False,
function_logical_ids: Optional[Tuple[str, ...]] = None,
no_reload: bool = False,
) -> None:
"""
Initialize the class with SAM template data. The SAM template passed to this provider is assumed
Expand Down Expand Up @@ -933,7 +934,10 @@ def __init__(
self.is_changed = False
self._observer = FileObserver(self._set_templates_changed)
self._observer.start()
self._watch_stack_templates(stacks)
if no_reload:

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Bug: Observer is still started even when no_reload=True

The FileObserver is unconditionally created and started on the lines above:

self._observer = FileObserver(self._set_templates_changed)
self._observer.start()

But the no_reload guard only skips _watch_stack_templates(). This means the observer thread is still spawned and running (consuming resources) even though nothing will ever be watched. This partially defeats the purpose of the flag — on Windows, even an idle FileObserver thread may interact with the file system.

You should also skip self._observer.start() when no_reload=True, or better yet, skip creating the observer entirely:

if no_reload:
    LOG.debug("Hot reload is disabled (--no-reload). Skipping template file observer.")
    self._observer = None
else:
    self._observer = FileObserver(self._set_templates_changed)
    self._observer.start()
    self._watch_stack_templates(stacks)

⚠️ If you go this route, you'll need to guard self._observer.stop() in the stop() method as well (check for None).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🔴 Observer still started when no_reload=True: The FileObserver is created and self._observer.start() is called unconditionally on line 936, even when no_reload=True. This means a background thread is still running even though nothing is being watched.

Consider wrapping both the observer creation and start in the condition:

if no_reload:
    LOG.debug("Hot reload is disabled (--no-reload). Skipping template file observer.")
    self._observer = None
else:
    self._observer = FileObserver(self._set_templates_changed)
    self._observer.start()
    self._watch_stack_templates(stacks)

Note: you'd also need to guard self._observer.stop() in the stop() method with an if self._observer: check.

LOG.debug("Hot reload is disabled (--no-reload). Skipping template file observer.")
else:
self._watch_stack_templates(stacks)

@property
def stacks(self) -> List[Stack]:
Expand Down
10 changes: 7 additions & 3 deletions samcli/local/lambdafn/runtime.py
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ class WarmLambdaRuntime(LambdaRuntime):
warm containers life cycle.
"""

def __init__(self, container_manager, image_builder, observer=None, mount_symlinks=False, no_mem_limit=False):
def __init__(self, container_manager, image_builder, observer=None, mount_symlinks=False, no_mem_limit=False, no_reload=False):

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Nit: Line length exceeds 120 characters

This line is quite long. Consider wrapping the parameters:

def __init__(
    self, container_manager, image_builder, observer=None,
    mount_symlinks=False, no_mem_limit=False, no_reload=False,
):

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🟡 Line too long: This line is ~113 characters after adding no_reload=False. Consider wrapping the parameters:

def __init__(
    self, container_manager, image_builder, observer=None,
    mount_symlinks=False, no_mem_limit=False, no_reload=False
):

"""
Initialize the Local Lambda runtime

Expand All @@ -528,6 +528,7 @@ def __init__(self, container_manager, image_builder, observer=None, mount_symlin
self._function_configs = {}
self._containers = {}
self._container_lock = threading.Lock() # Thread-safe container creation
self._no_reload = no_reload

self._observer = observer if observer else LambdaFunctionObserver(self._on_code_change)

Expand Down Expand Up @@ -596,8 +597,11 @@ def create(
return container

# Create new container
self._observer.watch(function_config)
self._observer.start()
if self._no_reload:
LOG.debug("Hot reload is disabled (--no-reload). Skipping file observer for '%s'", function_path)
else:
self._observer.watch(function_config)
self._observer.start()

container = super().create(
function_config, effective_debug_context, container_host, container_host_interface, extra_hosts
Expand Down
45 changes: 45 additions & 0 deletions tests/unit/commands/local/lib/test_sam_function_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -2615,6 +2615,51 @@ def test_provider_stop_will_stop_all_observers(self, get_template_mock, extract_
self.file_observer.stop.assert_called_once()


class TestRefreshableSamFunctionProvider_no_reload(TestCase):
"""Tests for --no-reload flag in RefreshableSamFunctionProvider"""

def setUp(self):
self.parameter_overrides = {}
self.global_parameter_overrides = {}
self.file_observer = Mock()
self.file_observer.start = Mock()
self.file_observer.watch = Mock()

@patch("samcli.lib.providers.sam_function_provider.FileObserver")
@patch.object(SamFunctionProvider, "_extract_functions")
@patch("samcli.lib.providers.provider.SamBaseProvider.get_template")
def test_no_reload_skips_template_watch(self, get_template_mock, extract_mock, FileObserverMock):
"""When no_reload=True, template files should NOT be watched"""
FileObserverMock.return_value = self.file_observer
extract_mock.return_value = {}
template = {"Resources": {}}
get_template_mock.return_value = template
stack = make_root_stack(template, self.parameter_overrides)

RefreshableSamFunctionProvider(
[stack], self.parameter_overrides, self.global_parameter_overrides, no_reload=True
)

self.file_observer.watch.assert_not_called()

@patch("samcli.lib.providers.sam_function_provider.FileObserver")
@patch.object(SamFunctionProvider, "_extract_functions")
@patch("samcli.lib.providers.provider.SamBaseProvider.get_template")
def test_reload_enabled_watches_templates(self, get_template_mock, extract_mock, FileObserverMock):
"""When no_reload=False (default), template files should be watched"""
FileObserverMock.return_value = self.file_observer
extract_mock.return_value = {}
template = {"Resources": {}}
get_template_mock.return_value = template
stack = make_root_stack(template, self.parameter_overrides)

RefreshableSamFunctionProvider(
[stack], self.parameter_overrides, self.global_parameter_overrides, no_reload=False
)

self.file_observer.watch.assert_called()


class TestSamFunctionProvider_search_layer(TestCase):
root_stack_template = {
"Resources": {
Expand Down
57 changes: 57 additions & 0 deletions tests/unit/local/lambdafn/test_runtime.py
Original file line number Diff line number Diff line change
Expand Up @@ -2346,3 +2346,60 @@ def test_clean_runtime_containers_stops_emulator_container(self, log_mock):
emulator_container.stop.assert_called_once()
log_mock.debug.assert_called_with("Stopping durable functions emulator container")
self.assertIsNone(runtime._durable_execution_emulator_container)


class TestWarmLambdaRuntime_no_reload(TestCase):
"""Tests for --no-reload flag in WarmLambdaRuntime"""

def setUp(self):
self.manager_mock = Mock()
self.lambda_image_mock = Mock()
self.observer_mock = Mock()

@patch("samcli.local.lambdafn.runtime.LambdaContainer")
def test_no_reload_skips_observer_watch_and_start(self, LambdaContainerMock):
"""When no_reload=True, observer.watch and observer.start should NOT be called"""
container = Mock()
LambdaContainerMock.return_value = container

runtime = WarmLambdaRuntime(
self.manager_mock, self.lambda_image_mock, observer=self.observer_mock, no_reload=True
)
runtime._get_code_dir = Mock(return_value="/some/code/dir")

func_config = Mock()
func_config.full_path = "MyFunction"
func_config.packagetype = "Zip"
func_config.durable_config = None
func_config.env_vars.resolve.return_value = {}
func_config.layers = []
func_config.runtime_management_config = None

runtime.create(func_config)

self.observer_mock.watch.assert_not_called()
self.observer_mock.start.assert_not_called()

@patch("samcli.local.lambdafn.runtime.LambdaContainer")
def test_reload_enabled_calls_observer_watch_and_start(self, LambdaContainerMock):
"""When no_reload=False (default), observer.watch and observer.start should be called"""
container = Mock()
LambdaContainerMock.return_value = container

runtime = WarmLambdaRuntime(
self.manager_mock, self.lambda_image_mock, observer=self.observer_mock, no_reload=False
)
runtime._get_code_dir = Mock(return_value="/some/code/dir")

func_config = Mock()
func_config.full_path = "MyFunction"
func_config.packagetype = "Zip"
func_config.durable_config = None
func_config.env_vars.resolve.return_value = {}
func_config.layers = []
func_config.runtime_management_config = None

runtime.create(func_config)

self.observer_mock.watch.assert_called_once_with(func_config)
self.observer_mock.start.assert_called_once()

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Suggestion: Assert the container is still created successfully when no_reload=True

The tests verify observer behavior but don't assert that the container itself was still created and returned. Adding assertions like:

result = runtime.create(func_config)
self.manager_mock.create.assert_called_once()
self.assertIsNotNone(result)

would strengthen the test by verifying that skipping the observer doesn't break container creation.