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

fix: implemented temporary workaround suggested by TileDB. (#2251) #58

Merged
merged 3 commits into from
Sep 13, 2021

Conversation

metakuni
Copy link
Contributor

@metakuni metakuni commented Sep 13, 2021

Reviewers

Functional:
@bkmartinjr, @MDunitz

Readability:
@ebezzi


Changes

  • modify: TileDB-Py version 0.9.5 -> 0.10.0 plus workaround for threading issue

Reviewer Notes

  • Bruce provided a bug repro to TileDB which showed that the failure occurs on TileDB-Py versions 0.8.8 and above, including the current version, 0.10.0.
  • This temporary workaround was suggested by Seth from TileDB and refined by Bruce to avoid hitting the cached X and X_col_shift arrays.

@codecov
Copy link

codecov bot commented Sep 13, 2021

Codecov Report

Merging #58 (084de28) into main (3d279a5) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #58      +/-   ##
==========================================
+ Coverage   70.04%   70.05%   +0.01%     
==========================================
  Files         138      138              
  Lines       10922    10926       +4     
==========================================
+ Hits         7650     7654       +4     
  Misses       3272     3272              
Flag Coverage Δ
frontend 70.05% <100.00%> (+0.01%) ⬆️
javascript 70.05% <100.00%> (+0.01%) ⬆️
smokeTest ?
unitTest 70.05% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
server/data_cxg/cxg_adaptor.py 87.04% <100.00%> (+0.08%) ⬆️
data_cxg/cxg_adaptor.py 87.04% <0.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d279a5...084de28. Read the comment docs.

futures.append(
executor.submit(
_mean_var_sparse_ab,
adaptor.open_array("X"), row_selector_A, nA, row_selector_B, nB, cols))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

adaptor.open_array("X") is the actual workaround the rest is reformatting to placate flake8.

executor.submit(_mean_var_ab, matrix, row_selector_AB, row_selector_A_in_AB, row_selector_B_in_AB, cols)
executor.submit(
_mean_var_ab,
adaptor.open_array("X"), row_selector_AB, row_selector_A_in_AB, row_selector_B_in_AB, cols)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

adaptor.open_array("X") is the actual workaround the rest is reformatting to placate flake8.

@metakuni
Copy link
Contributor Author

Note, as discussed at the war room meeting, holding off on merging this workaround until 15:00 PT today til we hear back from TileDB regarding an ETA for a fix.

@metakuni metakuni marked this pull request as ready for review September 13, 2021 19:03
@metakuni metakuni marked this pull request as draft September 13, 2021 19:22
@@ -19,6 +19,6 @@ pandas>=1.0,!=1.1 # pandas 1.1 breaks tests, https://github.com/pandas-dev/pand
PyYAML>=5.4 # CVE-2020-14343
scipy>=1.4
requests>=2.22.0
tiledb>=0.9
tiledb==0.10.0
Copy link
Contributor Author

@metakuni metakuni Sep 13, 2021

Choose a reason for hiding this comment

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

Pinned at 0.10.0 (current version) instead of specifying >=0.10.0 since, under the covers in TileDB-Py, the embedded TileDB versions can also get incremented.

This seems safer to me, but if I'm wrong, please let me know!

Copy link
Contributor

Choose a reason for hiding this comment

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

this seems fine to me. I'm unclear why all versions are not pinned. For our internal repo (ie, single-cell-explorer), we have no need to support multiple versions, and can safely pin.

This is not true for the cellxgene repo, which needs to allow users some flexibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering about that too. 😄

Copy link
Contributor

@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 good to me, based solely on visual inspection.

@metakuni metakuni marked this pull request as ready for review September 13, 2021 23:00
@metakuni metakuni merged commit 4d94a26 into main Sep 13, 2021
@metakuni metakuni deleted the kkatsuya/tiledb-workaround branch September 13, 2021 23:22
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