Skip to content

fix: Indexing optimization in structure tool #1338

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

Conversation

chandrasekharan-zipstack
Copy link
Contributor

What

  • Below changes in tool-structure
    • Ensured indexing is done only for the required combination
    • Renamed some boolean variables for readability
    • Minor log improvments

Why

  • Indexing was previously done for all prompts in the project since the extraction was linked to it and any prompt could have any config
  • This was wasteful and only the unique combinations of non LLM adapters mattered here along with chunk size and chunk overlap
  • Helps display cleaner logs which can help improve the understanding of what's happening

How

  • Used a set to obtain the unique combinations and called indexing for that alone

Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)

  • Yes, since it refactored some code involving different possibly stale code flows

Notes on Testing

  • Tested the changes locally for some simple executions, due to time constraint - it was not tested robustly. Hoping to cover that as part of the test suite

Screenshots

Checklist

I have read and understood the Contribution Guidelines.

Copy link
Contributor

@harini-venkataraman harini-venkataraman left a comment

Choose a reason for hiding this comment

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

Left a minor comment, LGTM otherwise

Copy link
Contributor

github-actions bot commented Jun 5, 2025

filepath function $$\textcolor{#23d18b}{\tt{passed}}$$ SUBTOTAL
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_logs}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_cleanup}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_cleanup\_skip}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_client\_init}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_image\_exists}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_image}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_container\_run\_config}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_container\_run\_config\_without\_mount}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_run\_container}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_image\_for\_sidecar}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{runner/src/unstract/runner/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_sidecar\_container}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{TOTAL}}$$ $$\textcolor{#23d18b}{\tt{11}}$$ $$\textcolor{#23d18b}{\tt{11}}$$

Copy link

sonarqubecloud bot commented Jun 5, 2025

@chandrasekharan-zipstack chandrasekharan-zipstack merged commit 1175cf1 into main Jun 5, 2025
5 checks passed
@chandrasekharan-zipstack chandrasekharan-zipstack deleted the fix/avoid-unnecessary-indexing-in-structure-tool branch June 5, 2025 14:09
@chandrasekharan-zipstack chandrasekharan-zipstack restored the fix/avoid-unnecessary-indexing-in-structure-tool branch June 6, 2025 07:55
chandrasekharan-zipstack added a commit that referenced this pull request Jun 6, 2025
@chandrasekharan-zipstack chandrasekharan-zipstack deleted the fix/avoid-unnecessary-indexing-in-structure-tool branch June 6, 2025 07:58
chandrasekharan-zipstack added a commit that referenced this pull request Jun 6, 2025
* Revert "fix: Summarization fix for structure tool (#1349)"

This reverts commit 381afa5.

* Revert "fix: Indexing optimization in structure tool (#1338)"

This reverts commit 1175cf1.
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