Skip to content

Conversation

@PaulM5406
Copy link
Contributor

@PaulM5406 PaulM5406 commented Aug 7, 2025

Hey @TkTech,

Running chancy with queues configured with concurrency=1, I have observed that polling next job waits for polling_interval.

The fix we added to set the queue wake event on conditionlen(jobs) == maximum_jobs_to_poll was not working. I have a fix to make it work for asyncex if you don't mind to have a look.

Thanks !

@codecov
Copy link

codecov bot commented Aug 7, 2025

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 71.51%. Comparing base (4026904) to head (5f2224b).
⚠️ Report is 11 commits behind head on 24_4.

Files with missing lines Patch % Lines
chancy/executors/asyncex.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             24_4      #48      +/-   ##
==========================================
+ Coverage   71.26%   71.51%   +0.25%     
==========================================
  Files          55       55              
  Lines        2899     2907       +8     
==========================================
+ Hits         2066     2079      +13     
+ Misses        833      828       -5     
Flag Coverage Δ
unittests 71.51% <85.71%> (+0.25%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@TkTech TkTech added the enhancement New feature or request label Aug 8, 2025
@TkTech TkTech changed the base branch from main to 24_4 August 8, 2025 00:27
@TkTech TkTech requested a review from Copilot August 13, 2025 06:21
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a queue polling issue in chancy where jobs would unnecessarily wait for the polling_interval when using queues with concurrency=1. The main change replaces the original wake event mechanism that wasn't working properly with a new approach that wakes the queue when the executor transitions from full to having available slots.

  • Refactors the job polling logic to use executor.free_slots instead of calculating concurrency manually
  • Implements a new wake event mechanism in the AsyncExecutor that triggers when a task completes and the executor was previously full
  • Removes the previous wake event logic that was based on the number of jobs fetched

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
chancy/worker.py Simplifies job polling logic by using executor.free_slots and removes the ineffective wake event mechanism
chancy/executors/asyncex.py Adds a custom task completion callback that sets the wake event when executor transitions from full to available

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

task.add_done_callback(self._on_task_done)

def _on_task_done(self, task):
if self.free_slots == 0:
Copy link

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

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

The condition self.free_slots == 0 is checked before removing the completed job from self.jobs, so it will always be false when a job completes. The check should be self.free_slots == 1 to detect when the executor was exactly full before the job completed.

Suggested change
if self.free_slots == 0:
if self.free_slots == 1:

Copilot uses AI. Check for mistakes.
@ppsmilesi
Copy link
Contributor

Hey @TkTech, could this change be part of O.24.4 release as well ? thanks !

@PaulM5406 PaulM5406 force-pushed the wake-event-when-executor-not-full-anymore branch from 151b3c3 to 5f2224b Compare August 28, 2025 20:52
TkTech added a commit that referenced this pull request Aug 31, 2025
@TkTech
Copy link
Owner

TkTech commented Sep 1, 2025

This is an interesting one. We want to keep the worker busy, but we also do not want to necessarily poll every time the executor has cleared a job. That could be an incredible number of queries, especially on something like an async executor with ~1000 concurrent tasks. I added a regression test for this in #49, but still thinking over what the right fix is.

I think the "right" fix might be to make "eager polling" an optional setting, which causes the executor to aggressively fill itself. I plan on releasing #49 tomorrow with support for this.

@PaulM5406
Copy link
Contributor Author

This is an interesting one. We want to keep the worker busy, but we also do not want to necessarily poll every time the executor has cleared a job. That could be an incredible number of queries, especially on something like an async executor with ~1000 concurrent tasks. I added a regression test for this in #49, but still thinking over what the right fix is.

I think the "right" fix might be to make "eager polling" an optional setting, which causes the executor to aggressively fill itself. I plan on releasing #49 tomorrow with support for this.

True ! An optional setting might be good, so user can decide to enable it when concurrency is low and disable it when concurrency is high.

@TkTech TkTech mentioned this pull request Sep 1, 2025
@TkTech TkTech closed this Sep 3, 2025
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.

3 participants