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

Enable tests in cibuildwheel step #209

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

dudoslav
Copy link
Collaborator

@dudoslav dudoslav commented Jan 22, 2024

This PR aims to enable tests in cibuildwheel step. For now, windows tests are disabled and only Linux+MacOS tests are enabled. This change should not only install the produced wheel but also runs the tests in this repo.

Currently the pipeline fails on (for more detailed output https://github.com/dudoslav/TileDB-Vector-Search/actions/runs/7609245130/job/20720948938):

  =========================== short test summary info ============================
  FAILED ../../../project/apis/python/test/test_cloud.py::CloudTests::test_cloud_flat
  FAILED ../../../project/apis/python/test/test_cloud.py::CloudTests::test_cloud_ivf_flat
  ============ 2 failed, 35 passed, 59 warnings in 434.08s (0:07:14) =============
  ::endgroup::Testing wheel...

Edit 1.: Note that Python CI API workflow fails due to pypa/gh-action-pypi-publish#49

@dudoslav dudoslav self-assigned this Jan 22, 2024
Copy link

This pull request has been linked to Shortcut Story #39475: [vs] Enable tests in CIBUILDWHEEL for Vector-Search repo.

@dudoslav
Copy link
Collaborator Author

Latest CIBUILDWHEEL with tests: https://github.com/dudoslav/TileDB-Vector-Search/actions/runs/8470380414

@ihnorton
Copy link
Member

@jparismorgan
Copy link
Contributor

jparismorgan commented Mar 28, 2024

I don't quite understand these:

FAILED test/test_index.py::test_delete_invalid_index - TypeError: string indices must be integers
FAILED test/test_index.py::test_delete_index - TypeError: string indices must be integers

They point to this code in TileDB-Py:

    def Config(cfg_dict=None):
        """
        Builds a tiledb config setting the login parameters that exist for the cloud service
        :return: tiledb.Config
        """
        restricted = ("rest.server_address", "rest.username", "rest.password")
    
        if not cfg_dict:
            cfg_dict = dict()
    
        for r in restricted:
            if r in cfg_dict:
                raise ValueError(f"Unexpected config parameter '{r}' to cloud.Config")
    
        host = config.config.host
    
        cfg_dict["rest.server_address"] = host
        cfg = tiledb.Config(cfg_dict)
    
        if (
            config.config.username is not None
            and config.config.username != ""
            and config.config.password is not None
            and config.config.password != ""
        ):
            cfg["rest.username"] = config.config.username
            cfg["rest.password"] = config.config.password
        else:
>           cfg["rest.token"] = config.config.api_key["X-TILEDB-REST-API-KEY"]
E           TypeError: string indices must be integers

I'm guessing that config.config.api_key is not a dictionary? Would want to repro locally to investigate more.

But separately for these:

ERROR test/test_cloud.py::CloudTests::test_cloud_flat - ValueError: TILEDB_REST_TOKEN not set
ERROR test/test_cloud.py::CloudTests::test_cloud_ivf_flat - ValueError: TILEDB_REST_TOKEN not set
ERROR test/test_cloud.py::CloudTests::test_cloud_ivf_flat_random_sampling - ValueError: TILEDB_REST_TOKEN not set

I think we need to set TILEDB_REST_TOKEN on the - name: Build wheels job like we do at https://github.com/TileDB-Inc/TileDB-Vector-Search/blob/main/.github/workflows/ci-python.yml#L41:

        env:
          TILEDB_REST_TOKEN: ${{ secrets.TILEDB_CLOUD_HELPER_VAR }}

@dudoslav
Copy link
Collaborator Author

dudoslav commented May 2, 2024

@jparismorgan We actually do set that variable (TILEDB_REST_TOKEN) here:

https://github.com/TileDB-Inc/TileDB-Vector-Search/pull/209/files#diff-14b51e9e6321caf5d23b2253f2415daf475fdaa417c0aa8fe896c3a460f2d023R130

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.

None yet

3 participants