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 support for profiling with Scalene #60

Merged
merged 19 commits into from
Feb 14, 2024
Merged

Add support for profiling with Scalene #60

merged 19 commits into from
Feb 14, 2024

Conversation

chuckwondo
Copy link
Collaborator

Added a new input (scalene_args) to allow users to run the algorithm via Scalene to produce a profiling (mem+cpu) report. The input value is simply a string of command-line options to pass to scalene to launch the algorithm within the existing subset.sh run script.

In addition, added/modified the following to address various inconveniences along the way:

  • Combined/simplified conda lock usage to significantly speed up the build.sh script that installs deps into the conda environment
  • Added bin/generate-lock-file.sh script for conveniently regenerating the conda lock file after changes are made to either environment.yml or environment-dev.yml
  • Updated algorithm_config.yaml to match new structure/naming implemented on the MAAP API side
  • Added convenience scripts for managing algorithm registration, all of which use the updated algorithm_config.yaml file:
    • bin/describe-algorithm.sh
    • bin/register-algorithm.sh
    • bin/delete-algorithm.sh
  • Updated docs/MAAP_USAGE.md accordingly

Here are 2 example profile outputs in the ADE, both using the same algo inputs, but one is a "reduced" profile:

  • ~/shared-buckets/dschuck/gedi-subset/profile.html
  • ~/shared-buckets/dschuck/gedi-subset/profile-reduced.html

@@ -5,21 +5,30 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog], and this project adheres to
[Semantic Versioning].

## [0.6.2] - 2023-12-05
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, did you manually drop the dates from the release versions? or was that an auto-formatting? isn't against semantic versioning to included - because they seem useful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed them from the file for 2 reasons: (1) each is a guess based on when we think the actual release will be made (so it is possible to be wrong), and (2) they are redundant because github automatically adds the release date to the releases page

Copy link
Contributor

Choose a reason for hiding this comment

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

But that would require a user to go dig into the Github releases and cross reference. It's much nicer to just have it in the log for humans to read.

bin/build-dps.sh Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to Document what this file's purpose is, comment at top would suffice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the expected method for executing this script and it's relatives?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The delete-algorithm and register-algorithm scripts are referenced in CONTRIBUTING.md. The describe-algorithm script is not described because it is not really intended to be used directly. The other 2 scripts call it.

Copy link
Contributor

@wildintellect wildintellect left a comment

Choose a reason for hiding this comment

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

I made some suggestions and asked a few clarifying questions. Nothing is a blocker that I can see, but I would like the readability for regular users addressed before approving.

CONTRIBUTING.md Outdated Show resolved Hide resolved
@chuckwondo chuckwondo merged commit e1cf5d5 into main Feb 14, 2024
@jjfrench jjfrench deleted the issue57-scalene branch February 15, 2024 05:19
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.

3 participants