Basic doc changes #486
Basic doc changes #486
Conversation
| index_name: str | ||
| auto_refresh: bool | ||
|
|
||
| docs: List[Document] = Field(default_factory=list) |
There was a problem hiding this comment.
My preference is to separate the documents from the parameters as part of a separate-data-from-config principle. Does this change your design?
There was a problem hiding this comment.
I'd prefer to parse the HTTP request's Dict/Json to a predefined structure immediately.
There was a problem hiding this comment.
It doesn't get parsed immediately, because requests can come in with batch_size and processes
| ) | ||
| return weights | ||
|
|
||
| class Document(BaseModel): |
There was a problem hiding this comment.
Consider logically separating this from the config-oriented objects. For example, by putting this at the top of the file and then having a section dedicated for config (# ------ ADD DOCUMENTS CONFIG OBJECTS: ------)
There was a problem hiding this comment.
We need Document above AddDocsParam for typing
There was a problem hiding this comment.
Yep, that requirement shouldn't conflict with my comment
| [(None, 'error'), ("1511", 'error'), ("cool", 'result'), ("144451", "error")]), | ||
| ([{123: "bad", "_id": "12345"}, {"_id": "cool"}], [("12345", 'error'), ("cool", 'result')]), | ||
| ([{None: "bad", "_id": "12345"}, {"_id": "cool"}], [("12345", 'error'), ("cool", 'result')]), | ||
| # handle bad content |
There was a problem hiding this comment.
why are these cases removed?
There was a problem hiding this comment.
Sorry, it doesn't seem I added some updates to the PR description. Because we validate immediately after the HTTP request comes in, there are some validations (generally just Typing validation) that cannot be caught and then returned as an error. These are those tests. May be a problem with this change.
There was a problem hiding this comment.
Is there any way around this? Like doing pydantic validation on each doc individually? and then adding the relevant "doc failed to be indexed" message to the response?
| @@ -25,6 +28,10 @@ def check_keys(cls, values): | |||
|
|
|||
|
|
|||
| class MappingObject(BaseModel): | |||
| """Field level control over a field. Currently only supports multi-modal combinations | |||
|
|
|||
| See: https://docs.marqo.ai/*/API-Reference/mappings/ | |||
There was a problem hiding this comment.
That link doesn't work. Use https://docs.marqo.ai/latest/API-Reference/mappings/ for this purpose. For better future proofing, link it to a specific version: https://docs.marqo.ai/0.0.21/API-Reference/mappings/
There was a problem hiding this comment.
I didn't think that, but it also means the docs are instantly out of date. At least with this is it's a regex of a link (we should also get latest to work).
There was a problem hiding this comment.
Test it out. It works with latest but not with *. I think it's best if users can copy-paste the link directly into their browser.
Latest vs fixed version is a tradeoff with between these two scenarios:
- We use
latest. If we refactor our docs, this link will be dead - We use a specific version. This may seem out of date, but link will always work. There will also be a "you are looking at an outdated version of the docs message"
latest should be fine as it's just for developers' reference
There was a problem hiding this comment.
hmm, latest did not work originally, but does now.
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Have unit tests been run against this PR? (Has there also been any additional testing?)
Related Python client changes (link commit/PR here)
Related documentation changes (link commit/PR here)
Other information:
Please check if the PR fulfills these requirements