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

Feature: Start Marqo Docker with any flags #28

Merged
merged 77 commits into from
Jun 2, 2023

Conversation

vicilliar
Copy link
Contributor

  • Please check if the PR fulfills these requirements
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added / updated (for bug fixes / features)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Feature (for new kinds of tests)

  • What is the current behavior? (You can also link to an open issue here)
    There is currently no simple way to restart marqo within application tests with different flags / env vars set.

  • What is the new behavior (if this is a feature change)?

  • a new utility rerun_marqo_with_env_vars was created so marqo can be stopped/restarted mid-test with new configuration (replicas, preloaded models, max ef const, etc)
  • all startup scripts were modified to accept this input
  • new application tests added using this feature
  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    No

  • Other information:
    When using this utility, note that the env vars must be formatted a certain way:
    Ensure that:

  1. Flags are separate items from variable itself (eg, ['-e', 'MARQO_MODELS_TO_PRELOAD=["hf/all_datasets_v4_MiniLM-L6"]'])
  2. Strings (individual items in env_vars list) do not contain ' (use " instead)
    -> single quotes cause some parsing issues and will affect the test outcome

End users will not interact with this, only developers / QA on marqo, so I think this is acceptable.

@@ -29,6 +29,10 @@ setenv =
TESTING_CONFIGURATION = LOCAL_MARQO_OS
PYTHONPATH = {toxinidir}{/}tests{:}{toxinidir}
PATH = {env:PATH}{:}{toxinidir}{/}scripts
; this is set in case test needs to stop & rerun marqo.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were set for each testenv since setting them in the parent [testenv] doesn't work for some reason.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doing some research it looks like each of these runs as its own subprocess, which would explain why exported env vars don't carry across

@vicilliar
Copy link
Contributor Author

Can confirm these work when run on EC2 instances for the following environments:

  • testenv:py3-local_os
  • testenv:py3-dind_os
  • testenv:py3-arm64_local_os
  • testenv:py3-cuda_dind_os

For testenv:py3-s2search, see this: #30

@@ -29,6 +29,10 @@ setenv =
TESTING_CONFIGURATION = LOCAL_MARQO_OS
PYTHONPATH = {toxinidir}{/}tests{:}{toxinidir}
PATH = {env:PATH}{:}{toxinidir}{/}scripts
; this is set in case test needs to stop & rerun marqo.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doing some research it looks like each of these runs as its own subprocess, which would explain why exported env vars don't carry across

@vicilliar vicilliar merged commit a499b05 into mainline Jun 2, 2023
@vicilliar
Copy link
Contributor Author

manually running and monitoring pipelines:
image

@vicilliar
Copy link
Contributor Author

all tests working as expected:

everything passes except s2search_CI. This will be fixed with this issue: #30
image

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