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

Add profiler/benchmark module #583

Merged
merged 11 commits into from
Oct 8, 2021

Conversation

danielhuppmann
Copy link
Member

@danielhuppmann danielhuppmann commented Sep 29, 2021

Please confirm that this PR has done the following:

  • Tests Added
  • Documentation Added
  • Name of contributors Added to AUTHORS.rst
  • Description in RELEASE_NOTES.md Added

Description of PR

This PR adds a "profile" module collecting some utility code for benchmarking and profile tests that I used for working on #580. This module is not intended to be tested or executed automatically as part of the CI/CD

@danielhuppmann danielhuppmann marked this pull request as ready for review September 29, 2021 08:21
@codecov
Copy link

codecov bot commented Sep 29, 2021

Codecov Report

Merging #583 (abd65ba) into main (151e330) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #583   +/-   ##
=====================================
  Coverage   93.7%   93.7%           
=====================================
  Files         50      50           
  Lines       5339    5339           
=====================================
  Hits        5004    5004           
  Misses       335     335           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 151e330...abd65ba. Read the comment docs.

Copy link
Contributor

@phackstock phackstock left a comment

Choose a reason for hiding this comment

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

Looks good over all, some comments from my side.

@@ -0,0 +1 @@
.pymon
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the profile module need its own .gitignore?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought that it's better to keep this as separate as possible for now - maybe we'll find that another package like memory-profiler is actually better for our use case...

profile/README.md Outdated Show resolved Hide resolved

### Installation

In addition to an installation of **pyam** with all optional dependencies,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it not make sense to add this as an installation option to setup.py? Perhaps under EXTRA_REQUIREMENTS under the key benchmark or something along those lines?

Copy link
Member Author

Choose a reason for hiding this comment

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

Similar to the previous reply: I'd keep all changes related to profiling contained in this folder for now, to avoid that we switch to another profiling solution but have stray/outdated references in other parts of the package.

profile/README.md Outdated Show resolved Hide resolved
@danielhuppmann
Copy link
Member Author

Thanks for the suggestions, @phackstock: my strategy here was to keep all changes related to profiling contained within this folder until we have figured out whether pytest-monitor is indeed the best solution for our use case. I also did some more experiments with memory-profiler...

I suggest to merge this PR as is and then continue working on performance improvements and other benchmarking/profiling tests.

@phackstock
Copy link
Contributor

@danielhuppmann sounds like a sensible approach to me. Having a benchmarking suite as part of the main branch of pyam is a good thing for sure. From my side we are good to merge.
We just should keep in mind to eventually move the .gitignore and the dependencies into the main program once we have a stable benchmarking environment. Maybe it's worth putting a note into the README that the benchmarking is still a work in progress for now.

@danielhuppmann
Copy link
Member Author

Added a note to the module-readme per suggestion by @phackstock

@danielhuppmann
Copy link
Member Author

Merging as is this is only an exploratory, work-in-progress module with no interconnection to the existing functionality of the package.

@danielhuppmann danielhuppmann merged commit 68e1bed into IAMconsortium:main Oct 8, 2021
@danielhuppmann danielhuppmann deleted the profiler branch October 8, 2021 15:45
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.

2 participants