Anirban166 / Autocomment-atime-results

GitHub Action that automatically comments a plot and other atime-based results on PRs
https://github.com/marketplace/actions/autocomment-atime-results
0 stars 1 forks source link

display amount of time spent installing different package versions #36

Closed tdhock closed 1 month ago

tdhock commented 1 month ago

currently there are two times displayed

        echo -e "\nTime taken to finish the standard R installation steps: $(formatTime $SETUP_DURATION)" >> report.md
        echo -e "\nTime taken to run \`atime::atime_pkg\` on the tests: $(formatTime $ATIME_TESTS_DURATION)" >> report.md

it would be great if we could also display how much time is taken on installing different versions of the package, versus actually running/timing the benchmarks. (this is part of running atime_pkg, not standard R installation steps)

since https://github.com/tdhock/atime/commit/74d4bc18a5e27900b80fb5e8e79379e96652983f a file called install_seconds.txt is saved to disk.

@Anirban166 can you please implement reading that file, and display time taken during each of the three steps?

I suspect that installing different git versions takes much more time on CI than running test cases.

Anirban166 commented 1 month ago

it would be great if we could also display how much time is taken on installing different versions of the package, versus actually running/timing the benchmarks. (this is part of running atime_pkg, not standard R installation steps)

@Anirban166 can you please implement reading that file, and display time taken during each of the three steps?

  • standard R installation
  • installing different git versions of the R package
  • running test cases

Agreed, and sure!

I can extract the timings (using something like scan which you used in your tests) and then use R's cat to redirect the output (an aptly named environment variable) from stdout to $GITHUB_ENV as part of the current Rscript execution (or just simply/directly use shell utilities like grep for the information parsing part)

since https://github.com/tdhock/atime/commit/74d4bc18a5e27900b80fb5e8e79379e96652983f a file called install_seconds.txt is saved to disk.

Since the packages we install (atime included) use CRAN mirrors, I'm waiting for it to be on the platform (I'm guessing you uploaded it and that you are waiting for it to be updated there?) https://github.com/Anirban166/Autocomment-atime-results/blob/0346c41e7de6ca40e41bfd94491ff603266da2f8/action.yml#L46-L47

tdhock commented 1 month ago

ok new atime is on CRAN today, please try that.

tdhock commented 1 month ago

also what do you think about organizing the timings in a table?

install R install data.table versions run/plot 14 test cases
3 min 6 min 2 min
Anirban166 commented 1 month ago

also what do you think about organizing the timings in a table?

Great idea!

For the structure, I prefer to have headings and columns for both the phase we are measuring and their respective timings (or the long format)

Here is an example of what I went with - let me know your thoughts!

I believe you might prefer having shorter versions than 'minutes' and 'seconds', but then I'm wondering how you would like it:

tdhock commented 1 month ago

I think that table looks good thanks! however "Running atime::atime_pkg on tests" may be confusing because atime_pkg() also does the installing different package versions. please remove atime_pkg() and instead write something like "Run/plot performance tests"

Anirban166 commented 1 month ago

Done! (updated example) And you're right, it gets confusing since it's a combination of two things here now. Sending a PR soon!

Anirban166 commented 1 month ago

Do you think it's worth exporting/saving the number of test cases (N.tests) to be able to instead say something like 'Running and plotting the 14 test cases'? (including the # of tests can add some weightage, but I'm fine with either way)

tdhock commented 1 month ago

the number of test cases is saved, load RData file and use length(test.list)

Anirban166 commented 1 month ago

Thanks! (Since we now have it in the plot too, do you think it's still worth adding?)

tdhock commented 1 month ago

maybe later