Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

add async query to improve latency #62

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open

add async query to improve latency #62

wants to merge 38 commits into from

Conversation

aditya1503
Copy link
Contributor

@aditya1503 aditya1503 commented Mar 26, 2025

Add async query to improve latency

NEW:
FINBENCHBACKUPLOCAL_agilityv1_latency_plots(2)

Screenshot 2025-03-26 at 5 52 03 PM

OLD:

FINBENCHBACKUPLOCAL_agilityv1_latency_plots(1)

Screenshot 2025-03-26 at 5 51 46 PM

@aditya1503 aditya1503 requested a review from elisno March 26, 2025 21:30
@aditya1503 aditya1503 mentioned this pull request Mar 27, 2025
4 tasks
expert_answer = self._remediate(query)
if expert_answer == None:
self._project._sdk_client.projects.entries.add_question(
self._project._id, question=query,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If is_bad_response == True, and expert_answer = None, then there's extra work being done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make add_question async as well

Copy link
Member

Choose a reason for hiding this comment

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

why did you mark this resolved? This seems like a critical consideration to think about.

If is_bad_response == True, and expert_answer = None, then there may be extra compute being run in these cases. Need to time this implementation vs. original implementation over a bunch of cases where is_bad_response == True, and expert_answer = None

@jwmueller
Copy link
Member

Please ensure you've created at least 3+ codex projects from the same dataset where you run the queries from this dataset in a different order. And then you've verified that in each one, the results using this async logic exactly match the results using our original logic, including when some of the questions have been answered in the Codex Project.

Base automatically changed from validator to main March 27, 2025 22:01
Comment on lines 225 to 230
# TODO: Make this async as well in the future (right now it takes 8% of the time)
with ThreadPoolExecutor(max_workers=1) as executor:
executor.submit(
self._project._sdk_client.projects.entries.add_question,
self._project._id, question=query
).result()
Copy link
Member

Choose a reason for hiding this comment

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

Before merge, a SWE needs to confirm there isn’t some simple latency gain left on the table in this line. Or that at least, the proper backend Eng tickets are in place to reduce this line's latency.

This line should ideally be near instant, any backend operations shouldn’t block it client side (eg. actually organizing how the new query gets logged in the DB shouldn't block the client).

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants