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.

Sign up for GitHub

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

Jump to bottom

Performance testing infrastructure. #10117

Merged
merged 1 commit into from
Feb 13, 2021
Merged

Performance testing infrastructure. #10117

merged 1 commit into from
Feb 13, 2021

Conversation

jmchilton
Copy link
Member

@jmchilton jmchilton commented Aug 17, 2020 •
edited
Loading

No description provided.

@jmchilton jmchilton force-pushed the performance_tests branch 8 times, most recently from bedb635 to 64691f1 Compare August 21, 2020 15:58
@jmchilton jmchilton force-pushed the performance_tests branch 2 times, most recently from 0753f12 to bbaaacd Compare December 2, 2020 22:43
@hexylena
Copy link
Member

hexylena commented Dec 8, 2020 •
edited
Loading

I like the idea of using statsd but it seems excessive and I'm worried the telegraf scraping is at too wide for test cases and it seems like unnecessary infrastructure in the context of narrow API

telegraf/statsd does seem a bit excessive as you suggest, have you considered something like https://github.com/airspeed-velocity/asv/ ? I tested it out with gxadmin (not a normal use case) and it was pretty nice for per-commit/per-infra setup performance numbers.

@mvdbeek
Copy link
Member

mvdbeek commented Dec 8, 2020

That does look interesting, but the docs are a bit basic. Did you manage to do timings around log statements ? The benchmarks all seem to revolve around total runtime when asv runs the actual code, which doesn't seem like it would be applicable / detailed enough ?

@hexylena
Copy link
Member

hexylena commented Dec 8, 2020

Hey @mvdbeek. I didn't try and do timings around log statements, only around select function calls. I think it grew out of benchmarking individual numpy function calls which felt like a good fit for galaxy, but I didn't look closely at the PR and if you're doing it based off of the diff between two log statements, maybe it isn't such an easy solution.

That said, if you want to use their viz infrastructure, since you're already writing out JSON, you could just write something like their JSON format (ugly I know) and get the nice auto-generated website for free?

@jmchilton jmchilton force-pushed the performance_tests branch 2 times, most recently from 8d40a69 to 709b1c6 Compare December 10, 2020 20:47
@jmchilton jmchilton mentioned this pull request Dec 14, 2020
Open
@jmchilton
Copy link
Member Author

I spent some time looking at allure for another context, it wasn't exactly what I'd want for this but it might be able to get there with the right plugin and some bootstrap. The numpy benchmarking tool looks awesome though.

What this PR currently produces is comparisons of the PR branch versus the target that look like:

Screen Shot 2020-12-15 at 12 09 13 PM

Screen Shot 2020-12-15 at 12 08 59 PM

You can download these artifacts from the Github action right now. There is another version that is just the timings without the comparison that I wanted to include on all the tests - but it really slowed down the tests immensely and to the point where they didn't get close to finishing it looks like so I reverted that change in the latest commit and just did the PR vs target comparison for the performance tests instead of for all the API tests.

@jmchilton
Copy link
Member Author

Older version of this PR backed up (https://github.com/jmchilton/galaxy/pull/new/performance_tests_backup). Did a very stripped down rebase into a single commit just now.

@jmchilton jmchilton changed the title [WIP] Performance testing infrastructure. Performance testing infrastructure. Jan 4, 2021
@jmchilton jmchilton requested a review from jdavcs January 4, 2021 20:50
@jdavcs
Copy link
Member

jdavcs commented Jan 7, 2021

Can I runtest_workflow_framework_performance.py as a standalone test? I'm getting a logging error: ValueError: I/O operation on closed file - is this because I missed an option I should've set or because it's not supposed to be run separately?

@jmchilton
Copy link
Member Author

It works for me to just run it directly with pytest (https://gist.github.com/jmchilton/e6007becb21db94db4c392b861094f6d). Is there any more context around that error?

@jdavcs
Copy link
Member

jdavcs commented Jan 14, 2021

Thank you for the gist - that's exactly the output I got. I was referring to this error (which, I assume, is not relevant) - https://gist.github.com/jmchilton/e6007becb21db94db4c392b861094f6d#file-gistfile1-txt-L36
I'll review this tonight, or tomorrow at the latest.

jdavcs
jdavcs reviewed Feb 10, 2021
View reviewed changes
scripts/tests_markdown_compare.tpl
Comment on lines +5 to +8
| **Sum** *(ms)* | ``{{ has_metrics.values() | map(attribute="sum") | join("`` | ``") }}`` |
| **Median** *(ms)* | ``{{ has_metrics.values() | map(attribute="median") | join("`` | ``") }}`` |
| **Mean** *(ms)* | ``{{ has_metrics.values() | map(attribute="mean") | join("`` | ``") }}`` |
| **Standard Deviation** | ``{{ has_metrics.values() | map(attribute="stdev") | join("`` | ``") }}`` |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same formatting suggestion as in tests_markdown.tpl

jdavcs
jdavcs reviewed Feb 10, 2021
View reviewed changes
scripts/tests_markdown.py Outdated Show resolved Hide resolved
jdavcs
jdavcs reviewed Feb 10, 2021
View reviewed changes
scripts/tests_markdown.py Outdated Show resolved Hide resolved
jdavcs
jdavcs approved these changes Feb 10, 2021
View reviewed changes
Copy link
Member

@jdavcs jdavcs left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so very cool! (that is, once I figured out how to run the tests and generate the reports!) I'm very sorry it took me so long to review this - now it has to be rebased. All my comments are minor suggestions only.

@jmchilton jmchilton force-pushed the performance_tests branch 2 times, most recently from 26b47b7 to adcfc86 Compare February 12, 2021 23:10
@jmchilton jmchilton merged commit b353f9a into galaxyproject:dev Feb 13, 2021
@jmchilton
Copy link
Member Author

Exploiting that approve to merge without a final check because I need to show some progress at PI meeting Monday, but I just rebased with the above comments. Thanks for the detailed review @ic4f!

@jdavcs
Copy link
Member

jdavcs commented Feb 15, 2021

Thank you for putting all that work into addressing those minor comments! Again, sorry for the delayed review @jmchilton !

@jmchilton jmchilton added this to the 21.05 milestone Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdavcs jdavcs jdavcs approved these changes

Assignees
No one assigned
Labels
area/dependencies area/performance area/scripts area/testing/api area/testing/integration area/testing/selenium area/testing kind/enhancement
Projects
None yet
Milestone
21.05
Development

Successfully merging this pull request may close these issues.
5 participants
@jmchilton @hexylena @mvdbeek @jdavcs @galaxybot