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] Skip tests that use tiledb-py if not installed #2883

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nguyenv
Copy link
Member

@nguyenv nguyenv commented Aug 12, 2024

Issue and/or context:

This PR is separated out from #2752

Changes:

Skip tests that use tiledb-py if not installed

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.

@nguyenv thanks!!!

I'm putting this as request-changes just to put a "hold" on it -- no indication anything's necessarily wrong but rather to flag some things for conversation:

  • Will we be doing pip-install of tiledb-py within our CI jobs?
    • If so: may need some (light) handling around core versions in the case when
      • Core has bumped storage-format version
      • TileDB-Py has that new core available at PyPI and TileDB-SOMA Py does not (or vice versa)
    • Also if so: that means the tests you have here may go "uncovered" for a while until we restore pip-install of tiledb-py within our CI YAML -- ok as long as we understand/track what we're doing
    • If not:
      • We'll be losing test coverage of assertions we once made and now no longer can
      • How bad is this?? Were these tests "extra" all along? Is this high-risk coverage or low-risk coverage we'd be losing?

Since the core C++ API (and C API) are "full APIs" that means anything we can/do do in TileDB-Py we should be able to do ourselves -- I want us to at least have the conversation about what to do with things like

            assert A.schema.sparse
            assert not A.schema.allows_duplicates
            assert A.dim("foo").filters == [tiledb.ZstdFilter(level=3)]
            assert A.attr("bar").filters == [tiledb.ZstdFilter()]

-- namely: can we either:

  • Expose some of these things via libtiledbsoma -> pybind11/Rcpp -> tiledbsoma-py/tiledbsoma-r -> so we can still test them at the pytest level?
  • Or, maybe expose something only at the libtiledbsoma level (no bindings) and then move these test cases from pytest (or testthat for R) to ctest for libtiledbsoma

@nguyenv
Copy link
Member Author

nguyenv commented Aug 13, 2024

My idea for the latter conerns: When creating a SOMAArray in C++, we can store the PlatformConfig that's passed in to generate the ArraySchema. Then have a platform_config getter that we can bind in the APIs. In Python, we could return that as a JSON formatted string.

So essentially your first bullet point.

@nguyenv
Copy link
Member Author

nguyenv commented Aug 13, 2024

I think we should install tiledb-py when running CI. We could have a requirements_ci.txt that pulls everything from requirements_dev.txt with just the additional requirement for tiledb. But yes we'd have to make sure the version of tiledb-py it pulls is compatible with the libtiledbsoma in tiledbsoma-py. I don't know how to deal with that yet but can look into it.

@johnkerl
Copy link
Member

My idea for the latter conerns: When creating a SOMAArray in C++, we can store the PlatformConfig that's passed in to generate the ArraySchema. Then have a platform_config getter that we can bind in the APIs. In Python, we could return that as a JSON formatted string.

So essentially your first bullet point.

Hmm, I hadn't thought of that. TBH I don't think this is a good check -- it will be checking a copy of platform config stored somewhere, not checking against readback of the actual core schema that was created. We need to actually read the core schema and do these checks -- if we want to continue doing them at all ...

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