Skip to content

Fix session cleanup#1106

Merged
dreadatour merged 1 commit intomainfrom
fix-session-cleanup
May 18, 2025
Merged

Fix session cleanup#1106
dreadatour merged 1 commit intomainfrom
fix-session-cleanup

Conversation

@dreadatour
Copy link
Copy Markdown
Contributor

Caught an error:

$ python index.py
Exception ignored in atexit callback <function Session._global_cleanup at 0x1058b76a0>:
Traceback (most recent call last):
  File ".../python3.13/site-packages/datachain/query/session.py", line 198, in _global_cleanup
    if isinstance(obj, Session):  # Cleanup temp dataset for session variables.
ReferenceError: weakly-referenced object no longer exists

While running this simplest script:

import datachain as dc

dc.read_storage("s3://bucket/", anon=True).save("dataset")

@dreadatour dreadatour requested review from a team and Copilot May 18, 2025 17:35
@dreadatour dreadatour self-assigned this May 18, 2025
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances session cleanup by catching ReferenceError for finalized objects and logging any other exceptions during global cleanup.

  • Wraps obj.__exit__ in a try/except to skip finalized sessions without crashing.
  • Logs unexpected errors encountered while cleaning up sessions.
Comments suppressed due to low confidence (1)

src/datachain/query/session.py:197

  • Add a test case that simulates a finalized Session object triggering a ReferenceError to verify that this cleanup path is correctly skipped and logged.
for obj in gc.get_objects():  # Get all tracked objects

Comment on lines +204 to +205
except Exception as e: # noqa: BLE001
logger.error(f"Exception while cleaning up session: {e}") # noqa: G004
Copy link

Copilot AI May 18, 2025

Choose a reason for hiding this comment

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

Catching all exceptions may hide unexpected errors; consider catching more specific exception types or using logger.exception to preserve the full traceback.

Suggested change
except Exception as e: # noqa: BLE001
logger.error(f"Exception while cleaning up session: {e}") # noqa: G004
except Exception: # noqa: BLE001
logger.exception("Unexpected exception while cleaning up session.") # noqa: G004

Copilot uses AI. Check for mistakes.
if isinstance(obj, Session):
# Cleanup temp dataset for session variables.
obj.__exit__(None, None, None)
except ReferenceError:
Copy link

Copilot AI May 18, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider adding a debug-level log statement here to record when a ReferenceError is caught, which can aid in future troubleshooting.

Suggested change
except ReferenceError:
except ReferenceError:
logger.debug("ReferenceError caught: Object has already been finalized.")

Copilot uses AI. Check for mistakes.
@codecov
Copy link
Copy Markdown

codecov Bot commented May 18, 2025

Codecov Report

Attention: Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.

Project coverage is 87.90%. Comparing base (41af6ff) to head (e6809c4).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/datachain/query/session.py 42.85% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1106      +/-   ##
==========================================
- Coverage   87.93%   87.90%   -0.03%     
==========================================
  Files         147      147              
  Lines       12655    12660       +5     
  Branches     1772     1772              
==========================================
+ Hits        11128    11129       +1     
- Misses       1091     1095       +4     
  Partials      436      436              
Flag Coverage Δ
datachain 87.83% <42.85%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dreadatour dreadatour merged commit 0db2282 into main May 18, 2025
34 of 35 checks passed
@dreadatour dreadatour deleted the fix-session-cleanup branch May 18, 2025 18:34
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.

3 participants