-
Notifications
You must be signed in to change notification settings - Fork 296
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
ENH: addition of carbon trackers to estimate compute costs #2834
Conversation
… into carbon-trackers
Thanks for opening this pull request! It looks like this is your first time contributing to fMRIPrep. 😄 "name": "Contributor, New FMRIPrep",
"affiliation": "Department of fMRI prep'ing, Open Science Made-Up University",
"orcid": "<your id>"
}, ```
Of course, if you want to opt-out this time there is no problem at all with adding your name later. You will be always welcome to add it in the future whenever you feel it should be listed. |
Hello @nikhil153! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2022-08-10 01:32:44 UTC |
Hi @effigies - finally got a chance to wrap-up this ENH we talked about at the brainhack! Let me know if you think of any improvements for simplifying usage. Thanks! |
… into carbon-trackers
@nikhil153 Thanks! I plan to look at this later this week for some initial feedback, but please bug me on Tuesday (holiday Monday) if I fail. |
Hi @effigies - bugging you as asked :) Any updates? |
Hi @effigies - I am seeing a couple of new conflicts on this thread. Any suggestions / comments? |
Resolved conflicts. Had a quick read-through, and this seems reasonably constrained. We'll probably want to push the dependencies into an extra (they'll get installed in the Docker image, but they shouldn't be required to install the package). I'm running some unrelated tests, and I can take the opportunity to patch this in while running them to get a sense of how it works. |
Hi @effigies, sounds good, thanks so much! |
Running with
Is a 0W fallback expected? |
So Docker version won't work unless run with the |
Co-authored-by: Chris Markiewicz <[email protected]>
Co-authored-by: Chris Markiewicz <[email protected]>
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 approve this in principle. I'm not sure what to do with the scripts/
and carbontracker/
directory. It looks like these were for proof-of-concept and/or documentation patches. What's the end goal for these?
Thanks @effigies! |
I would probably go ahead and make a new top-level document, like Lines 12 to 22 in 53765b5
I would place it above Lines 37 to 47 in 53765b5
Lines 54 to 59 in 53765b5
|
@nikhil153 I merged the condensed result: b6c8e1b Documentation can be updated at your convenience in a new PR. |
@effigies Thanks so much! Really appreciate your help in this project! Curious to see carbon estimate across hardware and geolocations. |
Changes proposed in this pull request
This PR is in reference to #2481 issue.
Documentation that should be reviewed
All the code changes and additions are described in this README. In summary, following files are to be reviewed:
Files modified:
Files added
Usage
Carbon tracker feature can be used by specifying two optional arguments:
--track-carbon
--country-code "CAN"