-
Notifications
You must be signed in to change notification settings - Fork 764
Fix: CPU usage calculation with starttime, relying only on /proc/pid>/stat
#6731
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
base: master
Are you sure you want to change the base?
Conversation
…pid>/stat` Signed-off-by: Josua Carl <josua.carl@uni-tuebingen.de>
This comment was marked as resolved.
This comment was marked as resolved.
Documentation: Fixed comments to be flagged for removal Signed-off-by: Josua Carl <josua.carl@uni-tuebingen.de>
modules/nextflow/src/main/resources/nextflow/executor/command-trace.txt
Outdated
Show resolved
Hide resolved
Signed-off-by: Josua Carl <josua.carl@uni-tuebingen.de>
pditommaso
left a comment
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.
Hi @JosuaCarl, thank you for working on this fix.
I'd like to ask if you could narrow the scope of this change to focus specifically on the CPU calculation fix, rather than rewriting the entire logic structure. The current PR introduces several structural changes:
- A new nested function
nxf_observe_task_stats()with nameref variables - Reordering of the existing flow and variable declarations
- Moving the
trapstatement and other logic around
While the underlying fix for the CPU calculation (using starttime from /proc/pid/stat instead of /proc/stat) makes sense, the extensive restructuring makes this change hard to test and validate across the wide variety of (legacy) systems where Nextflow runs.
Could you please refactor this PR to make only the minimal changes necessary to fix the CPU calculation bug? This would help us:
- Review the actual fix more easily
- Reduce the risk of introducing regressions on different Linux distributions and versions
- Make it easier to bisect if issues arise later
Thanks!
…nested functions and preserving execution order Signed-off-by: Josua Carl <josua.carl@uni-tuebingen.de>
|
Hi @pditommaso , regarding your remarks:
|
|
Not sure we can accept in the current form |
|
Ok, why exactly? If you want me to address anything else or feel like your requested changes were not addressed appropriately, please don't hesitate to say so. |
As described in #6710,
%cpuseems to exceed the value of 100% per core. The following update addresses this by removing/proc/statas a source for the globally passed ticks, relying instead onstarttimein/proc/<pid>/statas a global clock.%cpu/cpus> 100% #6710