S2Search testing pipeline does not actually test S2Search

[vicilliar]

The issue:
The marqo github CI workflow to test S2Search doesn't actually connect to any S2Search URL, it just acts like DIND.
Proof: Observe this run that passed: https://github.com/marqo-ai/marqo/actions/runs/5107294728/jobs/9180084294#step:8:4868
It runs its own OpenSearch container.

There are several causes:

1. The current S2Search URL (github secret) is outdated and has already been torn down. There is a new URL we should use (supplied by @VitusAcabado )
2. The tox does not properly pass env variables (from running conf) to the setup script. I think this is because the env vars set from running conf in clone_marqo_repo.sh in [testenv] commands_pre do not get carried over to the startup scripts in the child envs. Because of this, the s2search startup script reads $S2SEARCH_URL as empty, and starts its own marqo-os.

I don't know if this way of running conf used to work before and just doesn't now, or if it was always like this. Worth having a chat about!

Proposed solution:

1. Remove any setting of env vars and running conf in clone_marqo_repo.sh
2. Transfer LOCAL_OPENSEARCH_URL export declaration to conf file, so all exports are in 1 place (1 source of truth)
3. In all startup scripts, call . "$(pwd)/conf" at the start, thus env vars are properly set before running marqo/marqo-os
4. In the marqo repo, add the working S2Search URL as secret
5. Add skip flag for some tests in the S2Search environment. Tests where we change the marqo configuration (like max replicas) will fail because of the internal S2Search limit. For example, I get this error:

marqo.errors.MarqoWebError: MarqoWebError: MarqoWebError Error message: {'message': "{'error': {'root_cause': [{'reason': 'provided number of number_of_replicas 4 exceeds your Marqo limit. Current limit for number_of_replicas is 1', 'type': 'marqo_error'}]}, 'status': 400}\nPlease create an issue on Marqo's GitHub repo (https://github.com/marqo-ai/marqo/issues) if this problem persists.", 'code': 'unhandled_backend_error', 'type': 'backend_error', 'link': ''}
E       status_code: 500, type: backend_error, code: unhandled_backend_error, link:

[vicilliar]

It's also worth noting that in S2Search tests, this test fails: TestDemo.test_readme_example

The error

rneg1 = mq.index("my-first-index").delete()
>       assert rneg1["acknowledged"] is True
E       AssertionError: assert 'true' is True

tests/api_tests/test_demos.py:95: AssertionError

[pandu-k]

Thanks Joshua!

1. Can you list the steps you took to get your tests to work?

2. That sounds like a good plan. It would be good if there was a way we can add a check to ensure that the env vars are loaded in properly, to prevent this happening in the future.

Perhaps something like this (pseudocode) :

# test an env var that should be set in every environment
# maybe LOCAL_OPENSEARCH_URL is not the most appropriate one 
if LOCAL_OPENSEARCH_URL is not set:
    raise Error ("LOCAL_OPENSEARCH_URL is not set! ensure that the `conf` file was read properly!")

This can be added to the Python API tests, so that it gets checked in every environment. Perhaps here: https://github.com/marqo-ai/marqo-api-tests/blob/mainline/tests/marqo_test.py

[vicilliar]

Thanks Pandu!

1. Sure,
   For ARM, the && was removed and -id tags added to detach the marqo-os docker container to ensure no hanging from the subprocess call.

docker rm -f marqo-os
docker run -id --name marqo-os -p 9200:9200 -p 9600:9600 -e "discovery.type=single-node" marqoai/marqo-os:0.0.3-arm &

For S2Search,
I just ran conf at the start of the start_s2search_backend.sh file like this:

export MARQO_API_TESTS_ROOT=$(pwd)
. "${MARQO_API_TESTS_ROOT}/conf"

That already makes the tests run properly with the S2Search cluster.

2. Agreed! At the moment the relevant env vars are:

MARQO_API_TESTS_ROOT
S2SEARCH_URL
LOCAL_OPENSEARCH_URL

I feel we should put the checks in the startup shell scripts instead, since they're run every time. Adding it to marqo_test.py won't check upon first startup (when tox runs the scripts)

Like this: if [ -z "$S2SEARCH_URL" ]; then raise some error