Skip to content

Add pull method to ExclusiveRemoteTask #1027

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

Merged
merged 3 commits into from
May 2, 2025
Merged

Conversation

jon-wurtz
Copy link
Contributor

Add a pull method to ExclusiveRemoteTask. The same as fetch except recursive after a time.sleep with a poll time of 10 seconds.

Copy link
Contributor

github-actions bot commented May 2, 2025

PR Preview Action v1.6.1
Preview removed because the pull request was closed.
2025-05-02 17:02 UTC

Copy link
Contributor

github-actions bot commented May 2, 2025

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
8455 7454 88% 0% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
src/bloqade/analog/task/exclusive.py 35% 🟢
TOTAL 35% 🟢

updated for commit: 5d0bc27 by action🐍

Copy link

codecov bot commented May 2, 2025

Codecov Report

Attention: Patch coverage is 16.66667% with 10 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/bloqade/analog/task/exclusive.py 16.66% 10 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Member

@weinbe58 weinbe58 left a comment

Choose a reason for hiding this comment

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

Do not use recursion with an unbounded depth this is very bad practice.

self._task_result_ir = self._http_handler.fetch_results(self._task_id)
else:
time.sleep(poll_interval)
self.pull(poll_interval)
Copy link
Member

Choose a reason for hiding this comment

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

You can do this with a loop do not use recursion for with an unknown depth of the call stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I was wondering about this. I thought it would be okay because of the slow poll time

@weinbe58 weinbe58 merged commit 887749a into main May 2, 2025
5 of 7 checks passed
@weinbe58 weinbe58 deleted the exclusive_access_pull2 branch May 2, 2025 17:01
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.

2 participants