Skip to content

[python] Conditionally apply ContextManager cleanup logic#4426

Open
XanthosXanthopoulos wants to merge 2 commits intomainfrom
xan/conditionally_apply_context_management
Open

[python] Conditionally apply ContextManager cleanup logic#4426
XanthosXanthopoulos wants to merge 2 commits intomainfrom
xan/conditionally_apply_context_management

Conversation

@XanthosXanthopoulos
Copy link
Copy Markdown
Collaborator

Issue and/or context: SOMA groups are closing their members when running outside of ContextManager when they go out of scope while their member are still being accessed

Changes:
This PR changes the cleanup of members of groups:

  • When running under a ContextManager the behavior is unchanged
  • When not running under a context manager when a group goes out of scope its members are not being closed unless there is no reference to them

Notes for Reviewer:

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.38%. Comparing base (4de5c73) to head (6d0ae22).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4426      +/-   ##
==========================================
+ Coverage   84.33%   88.38%   +4.05%     
==========================================
  Files         151       68      -83     
  Lines       22004     7671   -14333     
  Branches       13        0      -13     
==========================================
- Hits        18557     6780   -11777     
+ Misses       3447      891    -2556     
Flag Coverage Δ
python 88.38% <100.00%> (-0.01%) ⬇️
r ?

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

Components Coverage Δ
python_api 88.38% <100.00%> (-0.01%) ⬇️
libtiledbsoma ∅ <ø> (∅)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Collaborator

@jp-dark jp-dark left a comment

Choose a reason for hiding this comment

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

This needs tests for the intended behavior.

assert all(r is results[0] for r in results)


def test_close_behavior() -> None:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This test would be cleaner using fresh objects instead of the pbmc3k. That dataset should only really be used if you need "real" data for the test.

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.

2 participants