Add gRPC ping check to is_live method

[sylvinho81]

This PR add gRPC ping check to is_live method.
Closes #1948

[orca-security-eu]

Orca Security Scan Summary

Status	Check	Issues by priority
Passed	Infrastructure as Code	0   0   0   0	View in Orca
Passed	SAST	0   0   0   0	View in Orca
Passed	Secrets	0   0   0   0	View in Orca
Passed	Vulnerabilities	0   0   0   0	View in Orca

[sylvinho81]

Hi @Shah91n

We opened a PR and noticed the CI pipeline is trying to download the artifact coverage-report-integration-embedded from workflow run #22132946685, but it doesn’t exist.

The artifacts that are actually available for that run are:

coverage-report-integration

coverage-report-mock_tests

coverage-report-test

weaviate-python-client-wheel

Could you clarify if coverage-report-integration-embedded is supposed to be uploaded in the workflow? Right now, this is causing the download step in the CI to fail.

[tsmith023]

Hi @sylvinho81, there's no need to worry about codecov!

[tsmith023]

These tests are great but there's one more case I'd like added to cover this change when init checks are skipped and one of the ports is wrong, which looks like:

def test_client_is_not_live_with_wrong_rest_port() -> None:
    with weaviate.connect_to_local(
        port=9000, # wrong
        skip_init_checks=True
    ) as client:
        assert not client.is_live()

def test_client_is_not_live_with_wrong_grpc_port() -> None:
    with weaviate.connect_to_local(
        grpc_port=90000, # wrong
        skip_init_checks=True
    ) as client:
        assert not client.is_live()

please could you add this case for sync and async then I'll merge!

[sylvinho81]

Hi @tsmith023

By looking at your suggested tests we think that they will fail and raise the exception like in the existing tests. If we specify a wrong port the connection will fail.

def test_connect_to_wrong_wcs() -> None:
    with pytest.raises(WeaviateStartUpError):
        weaviate.connect_to_wcs("does-not-exist", auth_credentials=WCS_CREDS, skip_init_checks=True)


def test_connect_to_wrong_local() -> None:
    with pytest.raises(expected_exception=WeaviateStartUpError):
        weaviate.connect_to_local("does-not-exist", skip_init_checks=True)


def test_connect_to_wrong_custom() -> None:
    with pytest.raises(expected_exception=WeaviateStartUpError):
        weaviate.connect_to_custom(
            "does-not-exist",
            http_port=1234,
            http_secure=False,
            grpc_host="does-not-exist",
            grpc_port=2345,
            grpc_secure=False,
            skip_init_checks=True,
        )

Instead we could just create the connection with the right ports and change it in the context. Something like this:

def test_client_is_not_live_with_wrong_rest_port() -> None:
    with weaviate.connect_to_local(skip_init_checks=True) as client:
        # Simulate a wrong REST port after connecting.
        client._connection.url = "http://localhost:9000"
        assert not client.is_live()

def test_client_is_not_live_with_wrong_grpc_port() -> None:
    with weaviate.connect_to_local(skip_init_checks=True) as client:
        # Simulate a wrong gRPC port after connecting.
        client._connection._connection_params.grpc.port = 90000
        # Clear channel and stub so ping fails and cleanup doesn't break
        client._connection._grpc_channel = None
        client._connection._grpc_stub = None
        assert not client.is_live()

Just let us know if this works and we add those tests.
Thanks

[tsmith023]

You're right, apologies for getting that so wrong! I do wonder whether skip_init_checks should truly skip all init checks so that it is less confusing 🤔

The second example looks good!

[sylvinho81]

Changes have been pushed, ready for review.

[weaviate-git-bot]

To avoid any confusion in the future about your contribution to Weaviate, we work with a Contributor License Agreement. If you agree, you can simply add a comment to this PR that you agree with the CLA so that we can merge.

beep boop - the Weaviate bot 👋🤖

PS:
Are you already a member of the Weaviate Forum?

[sylvinho81]

I agree with the CLA