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] Remove double open For SOMAArray reads #3293

Merged

Conversation

nguyenv
Copy link
Member

@nguyenv nguyenv commented Nov 4, 2024

Issue and/or context: #3472

This finishes up #3053 [sc-54744]

Changes:

  • Remove clib.SOMAArray.open in all SOMAArray derived class read calls
  • Pass query arguments coords, column names, result order, value filter, and platform config that were formerly passed to clib.SOMAArray.open or clib.SOMAArray.reset instead to TableReadIter and SparseNDArrayRead
  • _set_coords on the ManagedQuery rather than through SOMAArray
  • _arrow_table_reader and SparseTensorReadIterBase initialize a ManagedQuery object and set the query arguments listed above; set up the read; and submit reads and return results until the query is complete

[sc-59595]

@nguyenv nguyenv linked an issue Nov 4, 2024 that may be closed by this pull request
@johnkerl johnkerl changed the title [python] Remove Double Open For SOMAArray Reads [python] Remove double open For SOMAArray reads Nov 4, 2024
Copy link

codecov bot commented Nov 4, 2024

Codecov Report

Attention: Patch coverage is 93.58974% with 5 lines in your changes missing coverage. Please review.

Project coverage is 86.03%. Comparing base (c4fe227) to head (c469c0c).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3293      +/-   ##
==========================================
+ Coverage   85.83%   86.03%   +0.20%     
==========================================
  Files          55       55              
  Lines        6191     6174      -17     
==========================================
- Hits         5314     5312       -2     
+ Misses        877      862      -15     
Flag Coverage Δ
python 86.03% <93.58%> (+0.20%) ⬆️

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

Components Coverage Δ
python_api 86.03% <93.58%> (+0.20%) ⬆️
libtiledbsoma ∅ <ø> (∅)

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.

This is really really nice --a well-thought-out refactor, well executed. Just some minor mods requested. :)

apis/python/src/tiledbsoma/_dense_nd_array.py Outdated Show resolved Hide resolved
apis/python/src/tiledbsoma/_dense_nd_array.py Outdated Show resolved Hide resolved
apis/python/src/tiledbsoma/_read_iters.py Outdated Show resolved Hide resolved
@nguyenv nguyenv force-pushed the viviannguyen/sc-54744/python-soma-read-does-an-open-on-an-already branch from 89cd943 to 47c6f9f Compare November 5, 2024 00:31
@johnkerl johnkerl self-requested a review November 5, 2024 00:45
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.

🚢

Copy link
Member

@bkmartinjr bkmartinjr left a comment

Choose a reason for hiding this comment

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

This looks great. Two small items:

  1. platform_config handling - noted inline
  2. ManagedQuery GIL - the read path moves some logic up from C++ to Python -- we need to make sure the pybind entry points are all releasing GIL where appropriate. My quick skim is that it looks OK, but I suggest we benchmark after landing this to confirm.

@nguyenv nguyenv force-pushed the viviannguyen/sc-54744/python-soma-read-does-an-open-on-an-already branch 3 times, most recently from ac764f8 to 9124aff Compare November 13, 2024 20:11
@nguyenv nguyenv force-pushed the viviannguyen/sc-54744/python-soma-read-does-an-open-on-an-already branch 2 times, most recently from 37a03f2 to dc61711 Compare November 25, 2024 02:39
Copy link
Member

@bkmartinjr bkmartinjr left a comment

Choose a reason for hiding this comment

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

A bit of cleanup needed in _read_iters.py - comments inline

@nguyenv nguyenv force-pushed the viviannguyen/sc-54744/python-soma-read-does-an-open-on-an-already branch from eb6239c to 226f768 Compare November 25, 2024 21:31
@nguyenv nguyenv requested a review from bkmartinjr November 25, 2024 21:31
Copy link
Member

@bkmartinjr bkmartinjr left a comment

Choose a reason for hiding this comment

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

Couple of minor pythonic / lint items, but otherwise LGTM.

@nguyenv nguyenv force-pushed the viviannguyen/sc-54744/python-soma-read-does-an-open-on-an-already branch from 778def5 to 4b7f6bc Compare December 9, 2024 15:58
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.

🚢

Thank you @nguyenv !!!

@nguyenv nguyenv merged commit 1f81a86 into main Dec 9, 2024
15 checks passed
@nguyenv nguyenv deleted the viviannguyen/sc-54744/python-soma-read-does-an-open-on-an-already branch December 9, 2024 18:59
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.

[python/c++] The read method does an open on an already open array
3 participants