fix!: remove chunk_id property from Chunk class #3954
Conversation
cdoern
requested review from
ashwinb,
bbrowning,
ehhuang,
franciscojavierarceo,
hardikjshah,
leseb,
mattf,
raghotham,
reluctantfuturist,
slekkala1,
terrytangyuan and
yanxi0830
as code owners
meta-cla
bot
added
the
CLA Signed
|
this relates to conversation in #3895, fixes like this will make the API easier to split into a separate extensions package. |
| return str(uuid.UUID(hashlib.sha256(hash_input).hexdigest()[:32])) | ||
|
|
||
| @property | ||
| def chunk_id(self) -> str: |
There was a problem hiding this comment.
I think the bigger issue is its existence in the API file itself. The API should never have behavior like this at all. It should be data types and method declarations. Not implementation. Can we actually remove the usage of this property itself? You should use immutable datatypes -- something at the top level in the routers, etc. should say "oh there is no chunk id so I will generate one and then I will initialize an immutable object with this chunk id"
There was a problem hiding this comment.
yes sure, that makes more sense
cdoern
changed the title
cdoern
changed the title
| chunk_window = f"{i}-{i + len(toks)}" | ||
| chunk_id = generate_chunk_id(chunk, text, chunk_window) | ||
| chunk_metadata = metadata.copy() | ||
| chunk_metadata["chunk_id"] = chunk_id |
There was a problem hiding this comment.
need this in both places
chunk_id in the Chunk class executes actual logic to compute a chunk ID. This sort of logic should not live in the API spec. Instead, the providers should be in charge of calling generate_chunk_id, and pass it to `Chunk`. this removes the incorrect dependency between Provider impl and API impl Signed-off-by: Charlie Doern <[email protected]>
3 participants
What does this PR do?
chunk_id in the Chunk class executes actual logic to compute a chunk ID. This sort of logic should not live in the API spec.
Instead, the providers should be in charge of calling generate_chunk_id, and pass it to
Chunk.this removes the incorrect dependency between Provider impl and API impl