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

update desi_proc timing logging #1003

Merged
merged 3 commits into from Oct 13, 2020
Merged

update desi_proc timing logging #1003

merged 3 commits into from Oct 13, 2020

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Sep 25, 2020

This PR uses the new desiutil.timer.Timer class to standardize timing in bin/desi_proc. It uses some features in desihub/desiutil#152, so don't merge this until that PR is finalized.

New options to desi_proc:

  • --starttime $(date +%s): log the time between this starttime and the beginning of the python script (srun & python interpreter launch overhead).
  • --timingfile TIMINGFILE: save timing information to a json file for later analysis. When running in batch, this file is saved for each exposure.

New script bin/plot_timing uses the timing information saved with --timingfile to make plots like this:
science-20200315-00055611

@akremin please take a look to see if (or how badly) this will conflict with your changes in the desi_proc_mod branch.

@sbailey
Copy link
Contributor Author

sbailey commented Sep 25, 2020

desihub/desiutil#152 has been merged, so this PR is now ready for full review and can be merged when ready

Copy link
Member

@akremin akremin left a comment

Choose a reason for hiding this comment

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

Two small comments. Both are considerations to take, but neither is required for merge. I will wait for comment before clicking approve, but these changes look good.

bin/desi_proc Outdated
@@ -296,24 +314,30 @@ if args.batch:
cmd = ' '.join(inparams)
cmd = cmd.replace(' --batch', ' ').replace(' --nosubmit', ' ')

timingfile = f'{jobname}-timing-$SLURM_JOBID.json'
Copy link
Member

Choose a reason for hiding this comment

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

Should this be using the arg.timingfile from the parser or at least checking for its existence before defining to default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point; I added a check to use args.timingfile here.

Default usage with --batch is to not specify --timingfile, and then let desi_proc pick a timing file name based upon the job name and slurm jobid; but if the user specifies --batch and a specific --timingfile, that will be used instead.

Copy link
Member

Choose a reason for hiding this comment

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

Great. I think this is the best approach even if its not meant to be used by end users. It's either this or documenting the use cases, and this is simpler and more flexible.

bin/desi_proc Outdated

from collections import OrderedDict
Copy link
Member

Choose a reason for hiding this comment

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

Some of these moved imports aren't picked up as the same. Is there a motivation behind the moves or was this a result of adding and removing code during development?

It isn't a blocking factor but it does increase the differences with master that will impact the upcoming rebase of the desi_proc_mod branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was mostly a side effect of rearranging imports to be able to time them properly (import time and grab time.time() ASAP; wait on MPI until after argparsing; wait on freeze_iers until we've got the timers going...) For now I reverted the order to close to the original with only the minimally necessary rearrangement.

At the same time, our import order has gotten a bit messy compared to our (perhaps undocumented?) typical order:

  • python standard libraries (os, sys, glob, ...)
  • 3rd party libraries (numpy, scipy, astropy, ...)
  • other desi packages (desiutil, desitarget, ...)
  • other modules from within this package (desispec.io, desispec.sky, ...)

We can standardize that in a later update.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for reverting. We can refactor right after rebase so you can capture the timing appropriately.

@sbailey
Copy link
Contributor Author

sbailey commented Sep 29, 2020

Thanks for review @akremin; I made updates in response to your comments. Just in case I introduced a typo, I'd like to double check this at NERSC when Cori is back before merging.

Copy link
Member

@akremin akremin left a comment

Choose a reason for hiding this comment

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

Feel free to merge after testing when Cori comes back online

@sbailey sbailey merged commit 30061e4 into master Oct 13, 2020
@sbailey sbailey deleted the timing branch October 13, 2020 23:39
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.

None yet

2 participants