-
Notifications
You must be signed in to change notification settings - Fork 35
Print Timestamp and Resource Usage #193
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
Open
cfsarmiento
wants to merge
42
commits into
main
Choose a base branch
from
print-timestamp-and-resource-usage
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 36 commits
Commits
Show all changes
42 commits
Select commit
Hold shift + click to select a range
af98438
Adding a print statement for compilation
cfsarmiento 9f5eb17
Moving the print to the real compilation stage
cfsarmiento 86bcbf9
using dprint
cfsarmiento d0fbe1e
Forcing the output of the print statements
cfsarmiento 232672e
Seeing if this works for seeing the prints
cfsarmiento 730a194
Seeing if taking it away from the context manager works
cfsarmiento 13654d3
Adding flush=True to dprint
cfsarmiento 820b336
Not using dprint
cfsarmiento 0f52bee
seeing if these changes get picked up
cfsarmiento 8aea687
Adding compilation start/end prints
cfsarmiento 19aecb7
Using prometheus to get static metric reads
cfsarmiento 9e4fbc9
Added peak usage reporting
cfsarmiento dfa7a68
Optimizing condition
cfsarmiento f9bb511
Adding some error handling
cfsarmiento fb0f3af
Accidentally used a set instead of a dictionary
cfsarmiento 15e953d
Disabling SSL
cfsarmiento 2a7d951
debug statement
cfsarmiento d5806af
Output formatting
cfsarmiento 3aed132
Removing debug statements
cfsarmiento 406b849
Rounding to 2 instead of 3
cfsarmiento 87e8714
Adding logging for inference
cfsarmiento d7f314a
Adding imports to support type annotations
cfsarmiento 3296767
Additional formatting
cfsarmiento 36756b7
More formatting
cfsarmiento 363f971
Updating README with instructions on Prometheus setup
cfsarmiento c8ec9ef
Cleanup
cfsarmiento 5bcddc3
Revert "Cleanup"
cfsarmiento 2f12a97
Delete log.txt
cfsarmiento b5f0d30
removing package
cfsarmiento 661871f
removing unneeded imports
cfsarmiento d8635b5
Making a function for repeated stage prints
cfsarmiento 6e7ea31
Using function in compilation stage
cfsarmiento d0c5494
fixing linting errors
cfsarmiento bd96102
Adding type annotations for profile
cfsarmiento 60013ae
More explicit inference start/end steps
cfsarmiento 5e7a5e0
Changing capitalization on inference run
cfsarmiento 53e41b6
Adding flag for reporting resource utilization
cfsarmiento b80377d
Fixing conditional
cfsarmiento ae5724a
Making sure we don't see pip show output
cfsarmiento c227601
Adding more graceful error handling
cfsarmiento dfd9bbe
Updating README with reporting instructions
cfsarmiento 0d0f08d
Fixing linting errors
cfsarmiento File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Prometheus is not installed here, it looks like p will be none. Further on in the script you use p in other functions. If p is none, does everything work properly, or are there None type errors?
Prometheus is used unconditionally no matter what. You may want to make this opt-in for the user with some sort of argument, then raise an error if it is not installed. Right now, the code fails, but simply prints a warning. It would be more clear if nothing even referenced the module if it was not requested and an explicit failure if it was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change it to not print out anything at all if it is not installed.
pbeing none is fine since all the metric reporting only happens if it isn't none and those functions are the only things that actually usep. I ran the script multiple times on my end to make sure that it ran as expected whether the package is installed or not. If you look above in the description, I added screenshots of expected output and you can see that if the package is installed, it will provide resource metrics, otherwise it just tells you what stage the script is at.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! My only suggestion then is to make the parameters for p optional in the type hints. This makes it clear to anyone using the functions that p is not required.