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

Add HNSW hyperparameters m, ef_construction to default index_settings config #386

Merged
merged 9 commits into from
Apr 3, 2023

Conversation

Jeadie
Copy link
Contributor

@Jeadie Jeadie commented Mar 10, 2023

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
(#206)

Have unit tests been run against this PR? (Has there also been any additional testing?)

  • Yep, see the comments.

Related Python client changes (link commit/PR here)

  • No. Although some parameters are provided as kwargs to Index.create(, I don't think these are important enough to increase the complexity of the interface.

Related documentation changes (link commit/PR here)

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)

@Jeadie Jeadie marked this pull request as draft March 10, 2023 02:21
@Jeadie Jeadie had a problem deploying to marqo-test-suite March 10, 2023 02:24 — with GitHub Actions Failure
@Jeadie
Copy link
Contributor Author

Jeadie commented Mar 10, 2023

Test run: https://github.com/marqo-ai/marqo/actions/runs/4380687872/jobs/7668033896

  • Failed on encoding error (known issue).

@Jeadie
Copy link
Contributor Author

Jeadie commented Mar 10, 2023

Will require PR to marqodocs to add these changes to index_settings.

@Jeadie Jeadie marked this pull request as ready for review March 20, 2023 23:04
@Jeadie Jeadie had a problem deploying to marqo-test-suite March 20, 2023 23:06 — with GitHub Actions Failure
@Jeadie
Copy link
Contributor Author

Jeadie commented Mar 20, 2023

Copy link
Contributor

@vicilliar vicilliar left a comment

Choose a reason for hiding this comment

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

Looks mostly good, unit tests failed though. Could you try merging mainline and rerunning unit tests?

@@ -56,6 +56,16 @@ class IndexSettingsField:
number_of_shards = "number_of_shards"
number_of_replicas = "number_of_replicas"

ann_parameters = "ann_parameters"
ann_method = "method"
ann_metric = "space_type"
Copy link
Contributor

Choose a reason for hiding this comment

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

why isn't this just ann_space_type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, see above.

@@ -94,21 +95,13 @@ def add_customer_field_properties(config: Config, index_name: str,
utils.generate_vector_name(field_name[0])): {
"type": "knn_vector",
"dimension": model_properties["dimensions"],
"method": {
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be "ann_method" for clarity, since it's handling ann parameters. but may not be important as this isn't exposed to users

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC, this is explicitly defined by Opensearch, we must follow their convention.

@Jeadie
Copy link
Contributor Author

Jeadie commented Mar 29, 2023

@Jeadie Jeadie had a problem deploying to marqo-test-suite March 29, 2023 06:39 — with GitHub Actions Failure
@Jeadie
Copy link
Contributor Author

Jeadie commented Mar 30, 2023

@Jeadie Jeadie had a problem deploying to marqo-test-suite March 30, 2023 01:42 — with GitHub Actions Failure
@pandu-k
Copy link
Collaborator

pandu-k commented Mar 30, 2023

OwenPendrighElliott and others added 3 commits March 31, 2023 09:21
* Updated readme with new examples from the documentation and reordered the examples to keep document deletion and index deletion at the bottom

* removed accidental duplication of example

* updated links in README examples to more stable versions
@Jeadie
Copy link
Contributor Author

Jeadie commented Mar 30, 2023

https://github.com/marqo-ai/marqo/actions/runs/4569953224

@Jeadie Jeadie temporarily deployed to marqo-test-suite March 30, 2023 23:31 — with GitHub Actions Inactive
Copy link
Contributor

@vicilliar vicilliar left a comment

Choose a reason for hiding this comment

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

looks good, passed tests

@pandu-k pandu-k merged commit 37b1231 into mainline Apr 3, 2023
@pandu-k pandu-k deleted the jack/issue-206 branch April 3, 2023 03:03
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.

4 participants