-
Notifications
You must be signed in to change notification settings - Fork 107
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
Remove JobQueueManager #6452
Remove JobQueueManager #6452
Conversation
e2034d9
to
b191198
Compare
Codecov Report
@@ Coverage Diff @@
## main #6452 +/- ##
==========================================
- Coverage 83.41% 83.36% -0.05%
==========================================
Files 343 342 -1
Lines 20806 20769 -37
Branches 944 944
==========================================
- Hits 17356 17315 -41
- Misses 3163 3166 +3
- Partials 287 288 +1
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
b191198
to
19d4067
Compare
@@ -49,6 +49,11 @@ | |||
logger = logging.getLogger(__name__) | |||
|
|||
LONG_RUNNING_FACTOR = 1.25 | |||
"""If STOP_LONG_RUNNING is true, realizations taking more time than the average |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized that these attribute docstrings are not standardized, and cannot be picked up by any tool.
while self.is_active() and not self.stopped: | ||
self.launch_jobs(pool_sema) | ||
self.launch_jobs(self._pool_sema) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parameter being an instance variable; can it be moved into the launch_jobs method directly instead of passed in as a parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it can. Well spotted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, we need it to retain the possibility to set it when running from _builder/_legacy.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, because the execute_queue_via_websockets use the one from legacy. Got it!
|
||
def getNumPending(self) -> int: | ||
return self._queue_manager.getNumPending() | ||
return self._job_queue.count_status(JobStatus.PENDING) # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for all the type: ignore
? Will this be fixed in a separate PR/issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are due to difficulties with integrating the typing with the C-code and the custom cwrap
(not Pybind). It will go away as soon as we have thrown away the C-code..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good, very nice!
while self.is_active() and not self.stopped: | ||
self.launch_jobs(pool_sema) | ||
self.launch_jobs(self._pool_sema) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, because the execute_queue_via_websockets use the one from legacy. Got it!
It was only a thin wrapper with no value left. Move its functionality down to the JobQueue object.
4875731
to
07b9e87
Compare
It was only a thin wrapper with no value left. Move its functionality down to the JobQueue object.
Issue
Resolves #6285
Approach
Short description of the approach
(Screenshot of new behavior in GUI if applicable)
Pre review checklist
Ground Rules),
and changes to existing code have good test coverage.
Pre merge checklist