Robustness changes#58
Merged
Merged
Conversation
…ce cycles
[CHANGED] get_package_name, log_args, log_versions and log_licenses retained
references to frames from inspect.currentframe() without releasing them,
creating reference cycles. Now del the frame and its parent.
[FIXED] _log_metadata now normalises a None value to the "UNKNOWN" sentinel.
Version lines for unresolvable packages are logged as
name==UNKNOWN rather than name==None
Reviewer's GuideImproves robustness and cleanliness of logging utilities by ensuring frames are cleaned up after inspection and normalizing missing version metadata to 'UNKNOWN', with tests added to lock in the new behavior. Sequence diagram for updated metadata logging with UNKNOWN normalizationsequenceDiagram
participant Caller
participant Logger
participant get_version_for_package
Caller->>Logger: log_versions(packages)
Logger->>Logger: _log_metadata(packages, get_version_for_package, caller_name, label)
loop packages_and_dependencies
Logger->>get_version_for_package: get_version_for_package(pkg)
get_version_for_package-->>Logger: value
alt [value is None]
Logger->>Logger: resolved = UNKNOWN
else [value is not None]
Logger->>Logger: resolved = value
end
Logger->>Logger: log_message(name==resolved, label)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In the new test
test_log_versions_unresolvable_version_logs_unknown, consider cleaning upsys.pathandsys.modules['delme_nover']in atry/finallyblock to avoid leaking the temporary module and path change into other tests. - For the new
inspect.currentframe()usages where you manuallydel frame, parent, you might want to wrap the body in atry/finally(as you did inget_package_name) so that the frames are always cleared even ifinspect.getargvaluesor similar calls raise.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new test `test_log_versions_unresolvable_version_logs_unknown`, consider cleaning up `sys.path` and `sys.modules['delme_nover']` in a `try/finally` block to avoid leaking the temporary module and path change into other tests.
- For the new `inspect.currentframe()` usages where you manually `del frame, parent`, you might want to wrap the body in a `try/finally` (as you did in `get_package_name`) so that the frames are always cleared even if `inspect.getargvalues` or similar calls raise.
## Individual Comments
### Comment 1
<location path="src/scitrack/__init__.py" line_range="552-558" />
<code_context>
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
</code_context>
<issue_to_address>
**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:
```python
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.
</issue_to_address>
### Comment 2
<location path="src/scitrack/__init__.py" line_range="107" />
<code_context>
+ return ""
+ return _installed_package_from_globals(parent.f_globals)
+ finally:
+ del frame, parent
</code_context>
<issue_to_address>
**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:
```python
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
```
```python
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.
</issue_to_address>
### Comment 3
<location path="tests/test_logging.py" line_range="372-381" />
<code_context>
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)
+ 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)
+
+
</code_context>
<issue_to_address>
**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:
```python
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.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+552
to
558
| 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: |
There was a problem hiding this comment.
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:
- Remove any conditional guards of the form
if caller_value is not None:(or equivalent) around theentries.appendfor the caller, so thatNonevalues are allowed through to the loop that appliesresolved = value if value is not None else "UNKNOWN". - Ensure that any other special-casing of
caller_namethat skips logging onNoneis removed, so the caller is treated consistently with package entries.
| return "" | ||
| return _installed_package_from_globals(parent.f_globals) | ||
| finally: | ||
| del frame, parent |
There was a problem hiding this comment.
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, parentFrom your comment, get_package_name already uses a correct try/finally pattern. To fully implement your suggestion:
- Locate all other usages of
inspect.currentframe()in this file, specifically inlog_args,log_versions, andlog_licenses. - For each, wrap the logic that uses
frame/parentin atryblock and placedel frame, parentin a correspondingfinallyblock, mirroring the pattern used inget_package_nameand the edits above. - Ensure there are no early returns or exceptions paths that bypass the
finallyblock so that frame cleanup is guaranteed.
Comment on lines
+372
to
+381
| 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) |
There was a problem hiding this comment.
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()- Ensure
import sysis present at the top oftests/test_logging.py, as this test now usessys.pathandsys.modules. If it's missing, addimport sysalongside the other imports.
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 by Sourcery
Improve robustness of package introspection and metadata logging, and ensure version logging never emits None values.
Bug Fixes:
Tests: