Skip to content

Fix test_mode string evaluation in BaseMixin#1529

Draft
Copilot wants to merge 5 commits intomasterfrom
copilot/fix-test-mode-string-evaluation
Draft

Fix test_mode string evaluation in BaseMixin#1529
Copilot wants to merge 5 commits intomasterfrom
copilot/fix-test-mode-string-evaluation

Conversation

Copy link
Contributor

Copilot AI commented Feb 13, 2026

Description

String "false" in dj.config["custom"]["test_mode"] evaluated as True because BaseMixin._test_mode returned raw values without type conversion. Any non-empty string is truthy in Python.

Changes:

  • src/spyglass/utils/mixins/base.py: Changed _test_mode property to use spyglass.settings.config.test_mode instead of directly reading dj.config. This ensures string-to-boolean conversion via existing str_to_bool().
  • tests/utils/test_dj_helper_fn.py: Added test_str_to_bool() covering truthy/falsy strings and edge cases.
  • tests/utils/test_mixin.py: Added test_test_mode_property_uses_settings() to verify boolean return type.

Before:

dj.config["custom"]["test_mode"] = "false"
mixin._test_mode  # Returns "false" (string) → bool("false") = True ❌

After:

dj.config["custom"]["test_mode"] = "false"
mixin._test_mode  # Returns False (boolean) via str_to_bool() ✓

Checklist:

  • N/A. If this PR should be accompanied by a release, I have updated the CITATION.cff
  • N/A. If this PR edits table definitions, I have included an alter snippet for release notes.
  • N/A. If this PR makes changes to position, I ran the relevant tests locally.
  • N/A. If this PR makes user-facing changes, I have added/edited docs/notebooks to reflect the changes
  • I have updated the CHANGELOG.md with PR number and description.
Original prompt

Problem

Fix issue #1528: When test_mode is set as a string value "false" in dj.config, the BaseMixin._test_mode property incorrectly evaluates it as True (since any non-empty string is truthy in Python). This causes Spyglass to run in test mode unintentionally, triggering warnings like accept_divergence called in test mode, returning False w/o prompt.

Proposed Solution

Modify the _test_mode property in src/spyglass/utils/mixins/base.py to use spyglass.settings.test_mode which already correctly handles string-to-boolean conversion using the str_to_bool() function.

Implementation Details

In src/spyglass/utils/mixins/base.py, the _test_mode property currently reads:

@property
def _test_mode(self) -> bool:
    """Return True if in test mode.

    Avoids circular import. Prevents prompt on delete.

    Note: Using @property instead of @cached_property so we always get
    current value from dj.config, even if test_mode changes after first access.

    Used by ...
    - BaseMixin._spyglass_version
    - HelpersMixin
    """
    import datajoint as dj

    # Check dj.config directly instead of importing module-level variable
    # which gets stale if load_config() is called after initial import
    return dj.config.get("custom", {}).get("test_mode", False)

Change it to:

@property
def _test_mode(self) -> bool:
    """Return True if in test mode.

    Avoids circular import. Prevents prompt on delete.

    Note: Using @property instead of @cached_property so we always get
    current value from settings, even if test_mode changes after first access.

    Used by ...
    - BaseMixin._spyglass_version
    - HelpersMixin
    """
    from spyglass.settings import config as sg_config

    return sg_config.test_mode

This ensures that the string-to-boolean conversion happens correctly via SpyglassConfig.test_mode property which uses str_to_bool().

Expected Behavior After Fix

  • When test_mode is set to string "false" or "False", it will correctly evaluate to boolean False
  • Users will receive proper prompts during ingestion
  • No spurious test mode warnings will appear

Testing

Verify that:

  1. Setting "test_mode": "false" (string) in config now correctly evaluates to False
  2. Setting "test_mode": false (boolean) in config continues to work
  3. Setting "test_mode": "true" (string) correctly evaluates to True
  4. Setting "test_mode": true (boolean) continues to work
  5. Existing tests continue to pass

Fixes #1528

This pull request was created from Copilot chat.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 2 commits February 13, 2026 23:46
- Changed _test_mode property to use spyglass.settings.config.test_mode instead of reading directly from dj.config
- This ensures proper string-to-boolean conversion via str_to_bool() function
- Added test to verify _test_mode property returns boolean value
- Added comprehensive unit test for str_to_bool function

Fixes #1528

Co-authored-by: samuelbray32 <24991442+samuelbray32@users.noreply.github.com>
Changed docstring to reference exact module path 'spyglass.settings.config.test_mode' instead of 'SpyglassConfig.test_mode'

Co-authored-by: samuelbray32 <24991442+samuelbray32@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix test mode evaluation for string value in BaseMixin Fix test_mode string evaluation in BaseMixin Feb 13, 2026
Copilot AI requested a review from samuelbray32 February 13, 2026 23:52
# Check dj.config directly instead of importing module-level variable
# which gets stale if load_config() is called after initial import
return dj.config.get("custom", {}).get("test_mode", False)
return sg_config.test_mode
Copy link
Member

Choose a reason for hiding this comment

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

Is this config a dictionary? I think this needs...

Suggested change
return sg_config.test_mode
return sg_config.get('test_mode', False)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Switched it to just import test_mode directly

CBroz1 added a commit to CBroz1/spyglass that referenced this pull request Feb 19, 2026
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.

Bug: test_mode string value in config causes ingestion to run in test mode

3 participants