Skip to content
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

Time full program runtime #34

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Time full program runtime #34

wants to merge 18 commits into from

Conversation

ElliottKasoar
Copy link
Contributor

@ElliottKasoar ElliottKasoar commented Nov 17, 2023

Changes to timing method to resolve #33

  • The most significant change is to use /usr/bin/time to time the full runtime of each benchmark program (in addition to date as an extra check), for a more reliable value than timing within each loop (or even within the program but outside of loops, due to asynchronous functions etc.).
    • Note: this approach may need further changes for testing on multiple nodes, but this is not currently an issue.
  • Removes the module deletion/creation timer, as this is not something that will realistically be called multiple times, and arguably a more meaningful idea of the effect can be obtained by varying N (e.g. running for N=1000 and N=10,000, with the module timings becoming increasingly insignificant).
  • Removes (and does not update) the large stride benchmarks from the benchmarking script
  • Adds hard-coded (for simplicity) way to use cuda flag in forpy via TorchScript model, allowing GPU comparisons
    • The flag will not work without USETS=1 on compile time, but I'm not sure it's worth investing more time into forpy

To do:

  • Tidy up Python e.g. device comments/setting ("cpu" isn't set as the default as described, and more the docstring is unclear)
  • Update Python utils to read slurm output file with wall times
  • Update notebook with new plots/results (resolve Track benchmarking #16?)
  • Update changes to reflect changes for fypp preprocessor

@ElliottKasoar ElliottKasoar marked this pull request as ready for review December 15, 2023 14:56
@TomMelt
Copy link
Member

TomMelt commented Dec 15, 2023

Thanks @ElliottKasoar. I will try running this on my machine first, before I review it 👍

@TomMelt
Copy link
Member

TomMelt commented Mar 18, 2024

Code looks good. I am working with @surbhigoel77 to reproduce the tests on CSD3 before we merge

@jatkinson1000
Copy link
Member

@surbhigoel77 @TomMelt Is there any update on this?
We are just working through the project board recapping open issues.
Thanks.

@jatkinson1000
Copy link
Member

@surbhigoel77 @TomMelt Is there any update on this? We are just working through the project board recapping open issues. Thanks.

@surbhigoel77 @TomMelt
Please could you give us an update here?
This is still listed as 'in progress' on the projects board, does it need reverting or closing?

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.

Calculate mean from larger time intervals Track benchmarking
3 participants