deepset-ai / deepset-ai/haystack

File-level and inline comments in breakpoint.py say snapshot saving is 'enabled by default' (it is disabled)

Open Beginner friendly
#12,643 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

P2
Dominant language
Python
Stars
26.6k
Forks
3.2k
Avg merge
1d 3h
Merged PRs (30d)
194

Description

Bug: file-level and inline comments in breakpoint.py claim snapshot saving is enabled by default (it is disabled)

Haystack version: 3.1.0rc0 (upstream de7b89c98)
Affected code: haystack/core/pipeline/breakpoint.py

  • File-level comment, line 19: # Environment variable to control pipeline snapshot file saving (enabled by default)
  • Inline comment, line 165: # Check if snapshot saving is enabled via environment variable (enabled by default)
Problem

The two comments in breakpoint.py say snapshot saving is enabled by default, but the implementation, the function's docstring, and the surrounding code all agree the default is disabled:

# haystack/core/pipeline/breakpoint.py:19
# Environment variable to control pipeline snapshot file saving (enabled by default)
HAYSTACK_PIPELINE_SNAPSHOT_SAVE_ENABLED = "HAYSTACK_PIPELINE_SNAPSHOT_SAVE_ENABLED"


# haystack/core/pipeline/breakpoint.py:29-38
def _is_snapshot_save_enabled() -> bool:
    """
    ...
    The environment variable HAYSTACK_PIPELINE_SNAPSHOT_SAVE_ENABLED controls whether
    pipeline snapshots are saved to files. By default (when the variable is not set),
    saving is disabled. Only "true" and "1" (case-insensitive) enable saving; any other value disables it.

    :returns: True if snapshot saving is enabled, False otherwise.
    """
    value = os.environ.get(HAYSTACK_PIPELINE_SNAPSHOT_SAVE_ENABLED, "false").lower()
    return value in ("true", "1")


# haystack/core/pipeline/breakpoint.py:165
# Check if snapshot saving is enabled via environment variable (enabled by default)
if not _is_snapshot_save_enabled():
    logger.debug("Pipeline snapshot file saving is disabled via HAYSTACK_PIPELINE_SNAPSHOT_SAVE_ENABLED env var.")
    return None

So we have three documented claims about the same setting, and they don't agree:

Location Claim Correct?
File-level comment (line 19) "enabled by default" WRONG
_is_snapshot_save_enabled docstring (line 33) "saving is disabled" correct
Inline comment (line 165) "enabled by default" WRONG
os.environ.get(..., "false") default (line 37) implicit: disabled correct
Log message (line 166) "is disabled" correct
_save_pipeline_snapshot docstring (line 134) "The default file saving behavior is disabled" correct
Test conftest fixture (test/core/pipeline/breakpoints/conftest.py) monkeypatch.setenv(..., "true") required to enable correct (confirms default is disabled)

Five of seven sources say the default is disabled; the two wrong comments are the outliers.

Why this matters
  1. Source-of-truth confusion. A user reading the source file would see "enabled by default" at the top and form a wrong mental model. They would expect snapshots to be saved automatically on the first breakpoint hit, then have to debug why nothing landed on disk. The actual behavior is the opposite.
  2. Future-maintainer risk. If a future contributor reads the misleading file-level comment, they might "correct" the code to match the comment (e.g., by changing the default in os.environ.get to "true"), which would silently change the user-facing behavior. The test fixture at test/core/pipeline/breakpoints/conftest.py:9 exists precisely because the default is disabled — but the comment erases that signal.
  3. Small but real. A two-character fix (change "enabled" to "disabled" in both places) plus a regression test pins the default and prevents a future regression.
Reproducer (verified locally against upstream/main de7b89c98)
import os
os.environ.pop("HAYSTACK_PIPELINE_SNAPSHOT_SAVE_ENABLED", None)

from haystack.core.pipeline.breakpoint import _is_snapshot_save_enabled
print(_is_snapshot_save_enabled())  # False

The function returns False (saving is disabled), contradicting the file-level and inline comments which say "enabled by default".

Proposed fix

Two one-word changes:

- # Environment variable to control pipeline snapshot file saving (enabled by default)
+ # Environment variable to control pipeline snapshot file saving (disabled by default)
-     # Check if snapshot saving is enabled via environment variable (enabled by default)
+     # Check if snapshot saving is enabled via environment variable (disabled by default)

No code change. The function, the env-var default, the docstring, and the tests already agree; only the two comments are wrong.

Suggested regression test
def test_default_snapshot_saving_is_disabled(monkeypatch):
    """The default behavior (HAYSTACK_PIPELINE_SNAPSHOT_SAVE_ENABLED unset) is DISABLED.

    A regression here would surprise users who read the (currently wrong) file-level
    comment and expected snapshot files to be written by default.
    """
    monkeypatch.delenv("HAYSTACK_PIPELINE_SNAPSHOT_SAVE_ENABLED", raising=False)
    from haystack.core.pipeline.breakpoint import _is_snapshot_save_enabled
    assert _is_snapshot_save_enabled() is False
Scope

Two one-word comment fixes + a 4-line regression test. No public API change. No backward-compat concern: the behavior is already correct; the fix just makes the comments match the behavior.

Alternatives considered
  • Remove the inline comment entirely. The function name _is_snapshot_save_enabled is self-documenting; the log message right below already says "is disabled". But the file-level comment is the user-facing source of truth and should be fixed.
  • Add a unit test for _is_snapshot_save_enabled without fixing the comments. The comments would still mislead. The full fix is two-character comments + a test.
  • Leave it as a PR nit and not file an issue. The risk of a future contributor "fixing" the code to match the comment is real but small. A filed issue + a test pins the behavior and removes the ambiguity.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with haystack/core/pipeline/breakpoint.py and compare the two comments with _is_snapshot_save_enabled and its existing default. Review test/core/pipeline/breakpoints/conftest.py, then run the breakpoint tests. Done means both comments say saving is disabled by default and regression coverage verifies the environment variable's unset behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend, testing
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
88/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.