-
Notifications
You must be signed in to change notification settings - Fork 330
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
Add LocalEventManager
#40
Conversation
7ce8ee8
to
c0ca5b3
Compare
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.
Pull request is neither linked to an issue or epic nor labeled as adhoc!
8ab7158
to
35d1be1
Compare
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.
Pull request is neither linked to an issue or epic nor labeled as adhoc!
35d1be1
to
57762d4
Compare
694f859
to
e6ca8db
Compare
) | ||
|
||
def _get_cpu_info(self: LocalEventManager) -> LoadRatioInfo: | ||
cpu_actual_ratio = psutil.cpu_percent() / 100 |
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.
- an explicit interval sounds like a good idea for
cpu_percent
- this won't include CPU usage of e.g. playwright-controlled browsers, right? Shouldn't we check child processes as well, like in the memory usage method?
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.
1 - sure thing, makes sense
2 - If I understand the doc of psutil correctly ... The psutil.cpu_percent
function measures the system-wide CPU utilization, not just the CPU usage by your specific process or its child processes. So there should be no need to check child processes. I'd say in this case, it's what we want.
Oh, and could we write some tests? |
e6ca8db
to
b5ca2a6
Compare
a2a0cca
to
188a028
Compare
Of course, as discussed previously, I'll prepare tests once the core components stabilize. The recurring task, event manager, and local event manager can be considered in a solid state, I'll provide tests for them in the following PRs. I will provide tests for the system status module later since I suppose there could be some changes when finishing the Snapshotter. |
Since Honza is sick, I am merging this, so that I can work on subsequent things. In case of further comments, I'll make another contribution. |
Ticket
LocalEventManager
#28Description
LocalEventManager
from https://github.com/apify/crawlee/blob/v3.7.3/packages/core/src/events/local_event_manager.ts.RecurringTask
as an alternative tobetterSetInterval
from@apify/utilities
.Test script