Skip to content

fix: Fix incorrect use of desired concurrency ratio#780

Merged
Pijukatel merged 2 commits into
masterfrom
desired_concurrency_ratio_fix
Dec 5, 2024
Merged

fix: Fix incorrect use of desired concurrency ratio#780
Pijukatel merged 2 commits into
masterfrom
desired_concurrency_ratio_fix

Conversation

@Pijukatel

@Pijukatel Pijukatel commented Dec 4, 2024

Copy link
Copy Markdown
Collaborator

Description

Concurrency ratio was not correctly used in Python version of autoscaled pool. Align it with the Javascript implementation of autoscaled pool.
Add test.

Issues

@github-actions github-actions Bot added this to the 104th sprint - Tooling team milestone Dec 4, 2024
@github-actions github-actions Bot added t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics. labels Dec 4, 2024
@Pijukatel Pijukatel added the bug Something isn't working. label Dec 4, 2024
@Pijukatel Pijukatel requested review from janbuchar and vdusek December 4, 2024 10:20
@Pijukatel Pijukatel marked this pull request as ready for review December 4, 2024 10:21
@vdusek vdusek changed the title fix: Fix incorrect use of desired concurrency ratio. fix: Fix incorrect use of desired concurrency ratio Dec 4, 2024

@vdusek vdusek left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, just one typo & probably wrong test function declaration.

Comment thread tests/unit/_autoscaling/test_autoscaled_pool.py Outdated
Comment thread tests/unit/_autoscaling/test_autoscaled_pool.py
Co-authored-by: Vlada Dusek <v.dusek96@gmail.com>

@vdusek vdusek left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's wait for Honza's approve as well, since he implemented this and he knows the code well.

@vdusek vdusek left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One more thing, does this solve #768 as well? If so, add it please to the description with closes.

@Pijukatel

Copy link
Copy Markdown
Collaborator Author

One more thing, does this solve #768 as well? If so, add it please to the description with closes.

I was considering that. That issue is not well described so in the end I guessed, that it might be other problem. Might be solved by this, but I doubt it and without further details I will not be able to confirm it with certainty.
I have seen current concurrency, minimum concurrency and desired concurrency all changing, but user described it is not changing.

@Pijukatel Pijukatel merged commit d1f8bfb into master Dec 5, 2024
@Pijukatel Pijukatel deleted the desired_concurrency_ratio_fix branch December 5, 2024 08:55
@janbuchar janbuchar mentioned this pull request Dec 6, 2024
Mantisus pushed a commit to Mantisus/crawlee-python that referenced this pull request Dec 10, 2024
### Description
Concurrency ratio was not correctly used in Python version of autoscaled
pool. Align it with the Javascript implementation of autoscaled pool.
Add test.
### Issues

- Closes: apify#759

---------

Co-authored-by: Vlada Dusek <v.dusek96@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working. t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

autoscaled_pool.desired_concurrency_ratio is useless

3 participants