Skip to content

Robustness changes #58

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.

Sign up for GitHub

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

Merged
GavinHuttley merged 2 commits into from
Jun 24, 2026
+52 −26
Merged

Robustness changes #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e91453f
MAINT: del frame references in introspection helpers to avoid referen…
GavinHuttley Jun 24, 2026
9549d71
MAINT: release frame refs and normalise None metadata values
GavinHuttley Jun 24, 2026
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
21 changes: 15 additions & 6 deletions src/scitrack/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,12 @@ def get_package_name(obj: object | None = None) -> str:

frame = inspect.currentframe()
parent = frame.f_back if frame is not None else None
if parent is None:
return ""
return _installed_package_from_globals(parent.f_globals)
try:
if parent is None:
return ""
return _installed_package_from_globals(parent.f_globals)
finally:
del frame, parent

@sourcery-ai sourcery-ai Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion (bug_risk): Use try/finally around inspect.currentframe() usage to guarantee cleanup on exceptions.

In log_args, log_versions, and log_licenses, del frame, parent only runs on the happy path; any exception before it will keep the frame alive and maintain the reference cycle. To match get_package_name and avoid leaks in long‑lived processes, please wrap each inspect.currentframe() usage in a try/finally so cleanup always occurs.

Suggested implementation:

    frame = inspect.currentframe()
    parent = frame.f_back if frame is not None else None
    try:
        if parent is None:
            return ""
        return _installed_package_from_globals(parent.f_globals)
    finally:
        del frame, parent
    frame = inspect.currentframe()
    parent = frame.f_back if frame is not None else None
    try:
        if parent is None:
            return ""
        return _installed_package_from_globals(parent.f_globals)
    finally:
        del frame, parent

From your comment, get_package_name already uses a correct try/finally pattern. To fully implement your suggestion:

  1. Locate all other usages of inspect.currentframe() in this file, specifically in log_args, log_versions, and log_licenses.
  2. For each, wrap the logic that uses frame/parent in a try block and place del frame, parent in a corresponding finally block, mirroring the pattern used in get_package_name and the edits above.
  3. Ensure there are no early returns or exceptions paths that bypass the finally block so that frame cleanup is guaranteed.



def _version_via_metadata(name: str) -> str | None:
Expand Down Expand Up @@ -516,6 +519,7 @@ def log_args(self, args: dict[str, object] | None = None) -> None:
frame = inspect.currentframe()
parent = frame.f_back if frame is not None else None
args = inspect.getargvalues(parent).locals if parent is not None else {}
del frame, parent

result = {
k: args[k]
Expand Down Expand Up @@ -545,8 +549,10 @@ def _log_metadata(
Unions ``caller_name``'s installed dependencies with ``packages``,
resolves each name via ``value_for``, then emits ``name==value``
lines under ``label`` with the caller first, the rest
alphabetical. Lookups happen eagerly so a failed resolution aborts
before any line is written.
alphabetical. A ``None`` value is normalised to the ``"UNKNOWN"``
sentinel so a line is never written as ``name==None``. Lookups
happen eagerly so a failed resolution aborts before any line is
written.
"""
caller_value: str | None = None
if caller_name:
Comment on lines +552 to 558

@sourcery-ai sourcery-ai Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion: Align docstring and behavior for None values, especially for caller_name.

The new docstring matches the packages loop behavior, but caller_name is still skipped when value_for(caller_name) returns None due to the caller_value is not None guard. If you want all None values to become name=="UNKNOWN", consider removing or relaxing that guard and relying solely on the resolved = value if value is not None else "UNKNOWN" normalization. Otherwise, if omitting the caller on None is intentional, please update the docstring to call out this special case so it stays accurate.

Suggested implementation:

        caller_value: str | None = None
        if caller_name is not None:
            caller_value = value_for(caller_name)
            entries.append((caller_name, caller_value))

To fully align behavior with the docstring (all None values normalized to "UNKNOWN"), you should:

  1. Remove any conditional guards of the form if caller_value is not None: (or equivalent) around the entries.append for the caller, so that None values are allowed through to the loop that applies resolved = value if value is not None else "UNKNOWN".
  2. Ensure that any other special-casing of caller_name that skips logging on None is removed, so the caller is treated consistently with package entries.

Expand Down Expand Up @@ -582,7 +588,8 @@ def _log_metadata(
entries.append((pkg, value_for(pkg)))

for name, value in entries:
self.log_message(f"{name}=={value}", label=label)
resolved = value if value is not None else "UNKNOWN"
self.log_message(f"{name}=={resolved}", label=label)

def log_versions(self, packages: list[str] | str | None = None) -> None:
"""logs the caller's package, its installed dependencies, and named packages
Expand Down Expand Up @@ -611,6 +618,7 @@ def log_versions(self, packages: list[str] | str | None = None) -> None:
if parent is not None
else ""
)
del frame, parent
self._log_metadata(
packages,
get_version_for_package,
Expand Down Expand Up @@ -644,6 +652,7 @@ def log_licenses(self, packages: list[str] | str | None = None) -> None:
if parent is not None
else ""
)
del frame, parent
self._log_metadata(
packages,
_license_for_package,
Expand Down
17 changes: 17 additions & 0 deletions tests/test_logging.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,23 @@ def test_tracks_versions_module(logfile):
assert expect in line, line


def test_log_versions_unresolvable_version_logs_unknown(logfile):
# a module with no version attribute -> version None normalised to UNKNOWN
pyfile = TEST_ROOTDIR / "delme_nover.py"
pyfile.write_text("answer = 42\n")
sys.path.append(str(TEST_ROOTDIR))
import delme_nover

LOGGER = CachingLogger(create_dir=True)
LOGGER.log_file_path = logfile
LOGGER.log_versions(delme_nover)
Comment on lines +372 to +381

@sourcery-ai sourcery-ai Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion (bug_risk): Clean up sys.path and sys.modules mutations in the new test to avoid cross-test interference.

This test mutates global state by appending to sys.path and importing delme_nover without cleanup, which can cause ordering-dependent failures in other tests. Please wrap the sys.path change in a try/finally and remove the path entry, and delete sys.modules['delme_nover'] after the test (guarding for KeyError) so it doesn’t leak global state.

Suggested implementation:

def test_log_versions_unresolvable_version_logs_unknown(logfile):
    # a module with no version attribute -> version None normalised to UNKNOWN
    pyfile = TEST_ROOTDIR / "delme_nover.py"
    pyfile.write_text("answer = 42\n")
    module_name = "delme_nover"
    path_entry = str(TEST_ROOTDIR)
    sys.path.append(path_entry)
    try:
        delme_nover = __import__(module_name)

        LOGGER = CachingLogger(create_dir=True)
        LOGGER.log_file_path = logfile
        LOGGER.log_versions(delme_nover)
        LOGGER.shutdown()
        lines = logfile.read_text().splitlines()
        assert any("delme_nover==UNKNOWN" in line for line in lines)
        assert not any("delme_nover==None" in line for line in lines)
    finally:
        # Clean up global state and temporary file to avoid cross-test interference
        if path_entry in sys.path:
            sys.path.remove(path_entry)
        sys.modules.pop(module_name, None)
        if pyfile.exists():
            pyfile.unlink()
  1. Ensure import sys is present at the top of tests/test_logging.py, as this test now uses sys.path and sys.modules. If it's missing, add import sys alongside the other imports.

LOGGER.shutdown()
pyfile.unlink()
lines = logfile.read_text().splitlines()
assert any("delme_nover==UNKNOWN" in line for line in lines)
assert not any("delme_nover==None" in line for line in lines)


def test_get_package_dependencies_not_installed(monkeypatch):
# unknown package -> empty dict (never raises)
def fake_requires(name):
Expand Down
40 changes: 20 additions & 20 deletions uv.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading