Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: Fix incorrect use of desired concurrency ratio #780

Merged
merged 2 commits into from
Dec 5, 2024
Merged

fix: Fix incorrect use of desired concurrency ratio #780

merged 2 commits into from
Dec 5, 2024

Conversation

Pijukatel
Copy link
Contributor

@Pijukatel Pijukatel commented Dec 4, 2024 •
edited
Loading

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 approved these changes Dec 4, 2024
View reviewed changes
Copy link
Collaborator

@vdusek vdusek left a comment

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.

tests/unit/_autoscaling/test_autoscaled_pool.py Outdated Show resolved Hide resolved
tests/unit/_autoscaling/test_autoscaled_pool.py Show resolved Hide resolved
vdusek
vdusek reviewed Dec 4, 2024
View reviewed changes
Copy link
Collaborator

@vdusek vdusek left a comment

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 reviewed Dec 4, 2024
View reviewed changes
Copy link
Collaborator

@vdusek vdusek left a comment •
edited
Loading

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
Contributor 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 mentioned this pull request Dec 4, 2024
Closed
@Pijukatel Pijukatel merged commit d1f8bfb into master Dec 5, 2024
23 checks passed
@Pijukatel Pijukatel deleted the desired_concurrency_ratio_fix branch December 5, 2024 08:55
@janbuchar janbuchar mentioned this pull request Dec 6, 2024
Closed
Mantisus pushed a commit to Mantisus/crawlee-python that referenced this pull request Dec 10, 2024
@Pijukatel @vdusek @Mantisus
fix: Fix incorrect use of desired concurrency ratio (apify#780)
8ea12d3 
### 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdusek vdusek vdusek approved these changes

@janbuchar janbuchar janbuchar approved these changes

Assignees

@Pijukatel Pijukatel

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
Milestone
104th sprint - Tooling team
Development

Successfully merging this pull request may close these issues.

autoscaled_pool.desired_concurrency_ratio is useless
3 participants
@Pijukatel @janbuchar @vdusek