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 get similar document method #645

Conversation

the-sinner
Copy link
Contributor

Pull Request

Related issue

Fixes #642

What does this PR do?

  • Add get similar document method

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@curquiza curquiza self-requested a review June 6, 2024 13:46
@curquiza curquiza added the enhancement New feature or request label Jun 6, 2024
Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

Thank you very much for your PR @the-sinner 🙏 ❤️


class SimilarDocumentsQuery
{
private int|string|null $id = null;
Copy link
Member

Choose a reason for hiding this comment

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

Why null? this parameter is mandatory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix this

$this->index->waitForTask($promise['taskUid']);
}

public function testBasicSearchWithFilters(): void
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function testBasicSearchWithFilters(): void
public function testBasicSearchWithSimilarDocuments(): void

->setId($documentId)
);

self::assertNotNull($response);
Copy link
Member

@curquiza curquiza Jun 6, 2024

Choose a reason for hiding this comment

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

Can we check we have some expected fields in the answer (specific to similar documents query, like id) even if we don't check the value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can you share the response format of this api, i could not find it.
i tried running the rc1 locally, however i was getting "Cannot find embedder with name default"

Copy link
Member

Choose a reason for hiding this comment

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

I will ask my colleagues next week for having a basic tests with basic documents to make it test simply 😊

Choose a reason for hiding this comment

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

Hello!

To use the similar endpoints, you need to:

  1. enable the vectorStore experimental feature
  2. declare at least one embedder in your configuration

For quick sanity tests, the simplest is probably to declare a userProvided embedder.

The settings could be:

"embedders": {
            "manual": {
                "source": "userProvided",
                "dimensions": 3,
            }
        }

Then declare a set of documents that contain the _vectors.manual field, for example:

[
        {
            "title": "Shazam!",
            "release_year": 2019,
            "id": "287947",
            // Three semantic properties:
            // 1. magic, anything that reminds you of magic
            // 2. authority, anything that inspires command
            // 3. horror, anything that inspires fear or dread
            "_vectors": { "manual": [0.8, 0.4, -0.5]},
        },
        {
            "title": "Captain Marvel",
            "release_year": 2019,
            "id": "299537",
            "_vectors": { "manual": [0.6, 0.8, -0.2] },
        },
        {
            "title": "Escape Room",
            "release_year": 2019,
            "id": "522681",
            "_vectors": { "manual": [0.1, 0.6, 0.8] },
        },
        {
            "title": "How to Train Your Dragon: The Hidden World",
            "release_year": 2019,
            "id": "166428",
            "_vectors": { "manual": [0.7, 0.7, -0.4] },
        },
        {
            "title": "All Quiet on the Western Front",
            "release_year": 1930,
            "id": "143",
            "_vectors": { "manual": [-0.5, 0.3, 0.85] },
        }
    ]

The example are from the the engine test file for the similar endpoints.

can you share the response format of this api, i could not find it.

You're right I added the response format to the public usage page, please feel free to ask for more information as needed ☀️

@curquiza
Copy link
Member

curquiza commented Jun 6, 2024

@the-sinner do you think it would be relevant to create a SimilarDocumentsSearchResult file like https://github.com/meilisearch/meilisearch-php/blob/main/src/Search/SearchResult.php?

@curquiza
Copy link
Member

@the-sinner can you update tests according to dureuill message? 😇

@the-sinner the-sinner force-pushed the bump-meilisearch-v1.9.0 branch 2 times, most recently from eb312ed to 7e95ffb Compare June 10, 2024 15:36
@the-sinner
Copy link
Contributor Author

@curquiza i have added the test case shared by dureuill in the code.

@curquiza curquiza linked an issue Jun 11, 2024 that may be closed by this pull request
2 tasks
Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

Thank you @the-sinner

There is an issue with the tests

Capture d’écran 2024-06-11 à 10 05 06

@the-sinner
Copy link
Contributor Author

found the issue,
I am using Union types, but they are only supported after 8.0. ref

@the-sinner
Copy link
Contributor Author

@norkunas @curquiza
For removing the union type, there are two ways:

  1. make id string and only accept string values
  2. remove type from it and manually check the type while assigning the value

which one do you think would be better ?

In the reference link provided, the given example is using string ids, however the linked documentation allows both string and integer keys.

@norkunas
Copy link
Collaborator

Use phpdoc and that's all. People should respect the contract in the same way

src/Contracts/SimilarDocumentsQuery.php Show resolved Hide resolved
src/Contracts/SimilarDocumentsQuery.php Show resolved Hide resolved
src/Search/SimilarDocumentsSearchResult.php Outdated Show resolved Hide resolved
src/Search/SimilarDocumentsSearchResult.php Outdated Show resolved Hide resolved
src/Search/SimilarDocumentsSearchResult.php Outdated Show resolved Hide resolved
}

/**
* @param string[] $attributesToRetrieve an array of attribute names to retrieve
Copy link
Collaborator

@norkunas norkunas Jun 14, 2024

Choose a reason for hiding this comment

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

Suggested change
* @param string[] $attributesToRetrieve an array of attribute names to retrieve
* @param list<non-empty-string> $attributesToRetrieve an array of attribute names to retrieve

}

/**
* @param string[][] $filter an array of arrays representing filter conditions
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this shape be more specific?

$this->id = $id;
}

public function setOffset(?int $offset): SimilarDocumentsQuery
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function setOffset(?int $offset): SimilarDocumentsQuery
/**
* @param non-negative-int $offset
*/
public function setOffset(?int $offset): SimilarDocumentsQuery

return $this;
}

public function setLimit(?int $limit): SimilarDocumentsQuery
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function setLimit(?int $limit): SimilarDocumentsQuery
/**
* @param positive-int $limit
*/
public function setLimit(?int $limit): SimilarDocumentsQuery

return $this;
}

public function setEmbedder(string $embedder): SimilarDocumentsQuery
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public function setEmbedder(string $embedder): SimilarDocumentsQuery
/**
* @param non-empty-string $embedder
*/
public function setEmbedder(string $embedder): SimilarDocumentsQuery

}

/**
* @return array SimilarDocumentsQuery converted to an array with non null fields
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still don't see shape defined :)

src/Contracts/SimilarDocumentsQuery.php Outdated Show resolved Hide resolved
src/Contracts/SimilarDocumentsQuery.php Outdated Show resolved Hide resolved
@curquiza curquiza requested a review from norkunas June 18, 2024 16:21
@curquiza
Copy link
Member

Test failing are expected and due to the new RC

@curquiza curquiza merged commit bbc1116 into meilisearch:bump-meilisearch-v1.9.0 Jun 18, 2024
6 of 11 checks passed
norkunas added a commit to norkunas/meilisearch-php that referenced this pull request Jul 10, 2024
* Add get similar documents method

* Remove null from rankingScoreThreshold

* Update src/Contracts/SimilarDocumentsQuery.php

Co-authored-by: Tomas Norkūnas <[email protected]>

* Update src/Contracts/SimilarDocumentsQuery.php

Co-authored-by: Tomas Norkūnas <[email protected]>

---------

Co-authored-by: Clémentine <[email protected]>
Co-authored-by: Tomas Norkūnas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v1.9.0] Get similar documents
4 participants