Add "performance_test" to the benchmarks #2847
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2847 +/- ##
==========================================
+ Coverage 95.94% 95.96% +0.02%
==========================================
Files 366 366
Lines 53611 53619 +8
==========================================
+ Hits 51439 51458 +19
+ Misses 2172 2161 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Pull Request Test Coverage Report for Build 9729113408Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9729246492Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9730630372Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9733225497Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9733424593Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9747623796Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
Very interesting. Thanks for putting this together. I have some concerns though.
- I don't see the benefit of the performance table of contents section having a
maxdepthof 2. It could just go along with the release notes or at least be brought down to a maxdepth of 1. - The testing platform and environments being on the main performance page suggests that this platform will be used for all series of tests and it won't be (unless you're volunteering your time and your machine for the next N years). I think it'd be better to have that information maybe per-test document OR run these tests on a specific cloud-based platforms. We already have benchmarks running on the European Weather Cloud.
- I'm not sure the different resampling strategies need to be tested and documented in Satpy. This information seems like it would be better for pyresample.
- We will need the code that generated all of this to be added to the repository and easily runnable. Preferably it would generate these tables automatically.
- I feel like these types of results are highly dependent on the CPU and available memory of a system (assuming this is the only thing the machine is doing at the time). So I'm not sure how useful it is to show this to a user and have them expect similar numbers.
- I think a graph, maybe even an embedded javascript one, would be easier to digest. I wouldn't have recommended this normally, but since the main argument for this being added is making it easier to understand how worker and chunk size work together I think it is important.
- Lastly, I'm not sure how long lasting and useful this will be. These results could vary wildly with CPU and memory on the system (as I mentioned), but also with version changed of Satpy and satpy's dependencies. There was discussion somewhere else (github or slack) about having a script to show how a users specific machine performs with these different values. That may be better in the long term to make available to users and to make the results "personal" and actionable since it is their system they're seeing the results for.
I understand this is a draft and I am not the only satpy maintainer, but I just wanted to give my thoughts on this before you spend too much time on it if it ends up not being merged.
|
I agree. Actually I'm also not sure how useful my results could be for other platforms (That's why I only post one of them). I do have the test scripts that can finally output a csv file just like the tables in this PR. I'm making some improvements for public use. Maybe we can just let the user run it and decide what's the best settings like you said. But where shall we put it? |
|
I‘ve made some changes. Now we can review the html report of the test. |
yukaribbba
changed the title
yukaribbba
changed the title
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
Pull Request Test Coverage Report for Build 9823636005Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
Pull Request Test Coverage Report for Build 9827365298Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9827748182Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9828713154Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
…karibbba/satpy into add_performance_tests_to_docs
|
@djhoese CI failed again. Anyway my part is done temporarily. Here's an example of the test report. |
Pull Request Test Coverage Report for Build 9853151991Details
💛 - Coveralls |
There was a problem hiding this comment.
I really didn't think you were going to put more work into this until we got more feedback from other maintainers. It looks like you've done a lot. It does look more useful this way, but it is still a lot. I guess I was hoping for a python script that could be called and provided command line options for all of these things. But please don't put more work toward this until others have weighed in with their opinions.
I see why the module was added to the docs and their are API docs since users are expected to run it themselves, but I don't like the extra dependencies that it requires in the docs building (especially matplotlib). Note that pandas shouldn't be an extra dependency since xarray should depend on it.
Lastly, our documentation is usually written with linux-based examples. You're running on Windows so it does give a different "look" to the documentation you've added.
| i = 0 | ||
| for chunk_size in self.chunk_size_opts: | ||
| for num_worker in self.worker_opts: | ||
| self.single_loop((chunk_size, num_worker, resampler), generate=generate) | ||
| i = i + 1 |
There was a problem hiding this comment.
| i = 0 | |
| for chunk_size in self.chunk_size_opts: | |
| for num_worker in self.worker_opts: | |
| self.single_loop((chunk_size, num_worker, resampler), generate=generate) | |
| i = i + 1 | |
| for chunk_size, num_worker in itertools.product(self.chunk_size_opts, self.worker_opts): |
I wonder if the final report could be put after the for loop and instead find a way to only do the sleep (maybe split the completed message and the 1-min rest message into two prints) if we aren't on the last round.
| dask.config.set(num_workers=num_worker) | ||
|
|
||
| try: | ||
| num_thread = os.environ["OMP_NUM_THREADS"] |
There was a problem hiding this comment.
FYI we almost never see a good improvement if this is over 1 or 2.
|
|
||
| 4. Do I have enough swap memory? | ||
| -------------------------------- | ||
| Some conditions or resamplers may consume a hell of physical memory and then swap. When both are at their limits, |
There was a problem hiding this comment.
We generally try to avoid language like this.
| Some conditions or resamplers may consume a hell of physical memory and then swap. When both are at their limits, | |
| Some conditions or resamplers may consume a lot of physical memory and even use swap. When both are at their limits, |
|
I did think about command line options but eventually I found it looks nasty to let users put all these options in command. Beside, there're some kwargs hard to input that way. But ok, I'll be waiting for others' thoughts. |
|
For dependencies, we can drop |
Basically I was thinking that the script/module wouldn't be added to the docs (no API docs) and therefore we wouldn't need any of its dependencies because sphinx would never import it to be documented. Alternatively, we could tell sphinx to mock its dependencies in |
|
Well for me reading the docs are always better than reading the description inside the codes. Thats why I embed it. But the doc thing is just secondary. First we gonna hear what others say. |
|
Sorry for being so late on this. A couple of comments/questions (I haven't checked the code in detail yet, so forgive me if I overlooked something):
|
To have a better understanding of the performance topics in FAQ, several tests are taken. This PR will give brief on them.