-
Notifications
You must be signed in to change notification settings - Fork 217
Print total runtime of all builds #5032
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
Conversation
easybuild/main.py
Outdated
| if build_option('ignore_test_failure'): | ||
| success_msg += "(with --ignore-test-failure) " | ||
| success_msg += "for %s out of %s" % (correct_builds_cnt, len(ordered_ecs)) | ||
| success_msg += f"for {correct_builds_cnt} out of {len(ordered_ecs)} in {time2str(datetime.now() - start_time)}" |
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.
Maybe we could use the same style as other time reporting eg for the steps of ... (took TIME)?
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.
Like this?
== Build succeeded for 5 out of 5
... (took 46 mins 13 secs)
Not sure of this format here could be confused for something else and it was easier. It would need a 2nd print_msg to have it match more closely the other messages:
== Build succeeded for 5 out of 5
== ... (took 46 mins 13 secs)
But no strong opinion, can change it if you think one is better (which one?)
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.
I'd be fine also having it on one line, maybe we could do something like
Build succeeded for 5 out of 5 (total 46 mins 13 secs)
Also no strong opinion on it (could always be adjusted later) but agree it would be nice to have the total runtime of the build as well.
Maybe for a separate PR, but related to this it might also be worth to get a total CPU/node time for parallel jobs
easybuild/main.py
Outdated
| if build_option('ignore_test_failure'): | ||
| success_msg += "(with --ignore-test-failure) " | ||
| success_msg += "for %s out of %s" % (correct_builds_cnt, len(ordered_ecs)) | ||
| success_msg += f"for {correct_builds_cnt} out of {len(ordered_ecs)} in {time2str(datetime.now() - start_time)}" |
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.
== Build succeeded for 5 out of 5 (total 46 mins 13 secs)
I was wondering how that is better than
== Build succeeded for 5 out of 5 in 46 mins 13 secs
but while that reads more fluently it has too many numbers next to each other to read in a quick glimpse and now I see better why you preferred the "...took" variant..
I'll add a colon too:
| success_msg += f"for {correct_builds_cnt} out of {len(ordered_ecs)} in {time2str(datetime.now() - start_time)}" | |
| success_msg += f"for {correct_builds_cnt} out of {len(ordered_ecs)} (total: {time2str(datetime.now() - start_time)})" |
Crivella
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.
LGTM
|
Going in, thanks @Flamefire! |
We have individual runtimes like
But often, especially when builds are submitted as jobs and/or with many builds, I'd like to see a total build time
This PR adds that:
I excluded the time for parsing cmdline params etc and included only the build itself, i.e.
build_and_install_software.That should enough as the rest of the time should be reasonably small compared to the actual build(s)