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 Scalene CPU+mem profiling #4

Closed
wants to merge 9 commits into from
Closed

Add Scalene CPU+mem profiling #4

wants to merge 9 commits into from

Conversation

chuckwondo
Copy link
Contributor

Here's what I did:

  • Added the Scalene profiler to this algorithm so that we can see how memory and CPU are used, beyond calls to time within the code. This produces a profiling report named profile.html in the algorithm's output directory.
  • Refactored for a bit of clarity and to address a few issues that were noted in code comments. Everything now works cleanly and consistently in both the ADE and DPS.
  • Added algorithm_config.yml to the repo to enable programmatic algorithm registration. Note that prior to each release, the value of algorithm_version should be changed appropriately, in sync with changing get_dem.__version__ as well, prior to registration, in order for a new version to be registered. Registration should likely occur after merging the bumped versions into develop and tagging develop with the version value as well.

Fixes #1

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@chuckwondo
Copy link
Contributor Author

BTW, I registered GET-DEM:scalene to test the changes, so if you want to give things a spin, choose that algo.

@chuckwondo
Copy link
Contributor Author

@nemo794, although the total amount of code is small, I made significant changes, so I'd be happy to pair up with you to go over them, if you like.

@nemo794
Copy link
Collaborator

nemo794 commented Mar 4, 2024

Hi @chuckwondo , That'd be great.

@chuckwondo
Copy link
Contributor Author

@nemo794, I've sent you a DM in the Slack ESA-NASA MAAP workspace regarding my availability.

@chuckwondo
Copy link
Contributor Author

Closing this PR after @nemo794 and I discussed the need to break it into smaller pieces, and also the need to wait for other PRs to land first, in order to support better coordination between NASA and ESA.

@chuckwondo chuckwondo closed this Mar 6, 2024
@wildintellect
Copy link
Collaborator

@chuckwondo can you be more specific, are you waiting for #2 to happen first?

@chuckwondo
Copy link
Contributor Author

Sorry, yes, waiting for #2 to happen first. Also, @nemo794 and I discovered that the call out to sardem via a system call does not allow scalene to capture profiling details, so I'll work on changing get-dem to call sardem directly via a function call, which should allow us to get a complete profiling report.

Once #2 lands, Sam and I will work on creating a small, focused PR to get scalene integrated with as few changes as possible, hopefully making it easier for ESA to pull in as well.

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.

Add Internal Benchmark tooling
3 participants