Skip to content
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.

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

[python] Make ExperimentAxisQuery inherit somacore, restore its .close #3493

Merged
merged 3 commits into from
Dec 20, 2024

Conversation

ryan-williams
Copy link
Member

@ryan-williams ryan-williams commented Dec 20, 2024

Issue and/or context:

  • #3307/somacore#244 moved ExperimentAxisQuery over from somacore, but mistakenly didn't inherit from the somacore.ExperimentAxisQuery ABC.
  • It also made ExperimentAxisQuery.close a no-op (per this thread).
  • #3359 removed ExperimentAxisQuery.close altogether, which should have triggered a pre-commit error since EAQ was no longer implementing the somacore ABC.
  • This corrects both issues. I also verified that adding the inheritance causes the missing close to fail pre-commit.

[sc-59686]

@ryan-williams ryan-williams marked this pull request as ready for review December 20, 2024 15:51
@johnkerl johnkerl changed the title make EAQ inherit somacore, restore EAQ.close [python] Make ExperimentAxisQuery inherit somacore, restore its .close Dec 20, 2024
Copy link
Member

@johnkerl johnkerl left a comment

Choose a reason for hiding this comment

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

Please restore test_query_cleanup from
https://github.com/single-cell-data/TileDB-SOMA/blob/1.14.5/apis/python/tests/test_experiment_query.py#L541-L567
but omitting the threadpool-is-None/not-None checks per
#3307 (comment)

We need unit-test coverage that we have a .close method on ExperimentAxisQuery

Copy link
Member

@johnkerl johnkerl left a comment

Choose a reason for hiding this comment

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

Thanks @ryan-williams !

apis/python/src/tiledbsoma/_query.py Outdated Show resolved Hide resolved
@johnkerl johnkerl merged commit 11ee34c into main Dec 20, 2024
10 checks passed
@johnkerl johnkerl deleted the rw/eaq branch December 20, 2024 17:36
github-actions bot pushed a commit that referenced this pull request Dec 20, 2024
….close` (#3493)

* make EAQ inherit somacore, restore EAQ.close

* restore `test_query_cleanup` sans threadpool checks

* Update apis/python/src/tiledbsoma/_query.py

Co-authored-by: John Kerl <[email protected]>

---------

Co-authored-by: John Kerl <[email protected]>
johnkerl added a commit that referenced this pull request Dec 20, 2024
….close` (#3493) (#3500)

* make EAQ inherit somacore, restore EAQ.close

* restore `test_query_cleanup` sans threadpool checks

* Update apis/python/src/tiledbsoma/_query.py



---------

Co-authored-by: Ryan Williams <[email protected]>
Co-authored-by: John Kerl <[email protected]>
Copy link

codecov bot commented Dec 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.32%. Comparing base (dcbdba2) to head (a0362b6).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3493      +/-   ##
==========================================
+ Coverage   86.27%   86.32%   +0.05%     
==========================================
  Files          55       55              
  Lines        6338     6340       +2     
==========================================
+ Hits         5468     5473       +5     
+ Misses        870      867       -3     
Flag Coverage Δ
python 86.32% <100.00%> (+0.05%) ⬆️

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

Components Coverage Δ
python_api 86.32% <100.00%> (+0.05%) ⬆️
libtiledbsoma ∅ <ø> (∅)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants