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

enable zproc.main to be called from other scripts #2391

Merged
merged 2 commits into from
Oct 11, 2024
Merged

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Oct 11, 2024

This PR provides two changes to desispec.scripts.zproc.main to enable it to be called from other scripts:

  • Return an integer error code instead of calling sys.exit directly. Previously even upon success, it would call sys.exit(0) instead of returning to the calling function. The command line script bin/desi_zproc already includes sys.exit(zproc.main()) so that did not need to be changed.
  • Generate the command line from input args or sys.argv. This is needed for the --batch option, which previously assumed the command line was coming only from sys.argv which isn't the case if a wrapper script is calling zproc.main with a list of strings.
    • Caveat: if the wrapper script calls zproc.main with an args.Namespace object, this will still incorrectly generate the command line from sys.argv. That's the same as what the current code does, i.e. this PR doesn't solve all cases, but it at least provides a workaround via supporting passing a list of strings to main.

This was motivated by Loa testing wanting to batch submit an individual tile/nights without having to call the command line script and re-import packages etc every time (slow). An alternative would have been to use desi_tile_redshifts and desi_healpix_redshifts instead, but I was mostly done with this update before realizing that from talking with @akremin, and I think these are useful functionality updates anyway.

@sbailey sbailey requested a review from akremin October 11, 2024 17:28
@coveralls
Copy link

coveralls commented Oct 11, 2024

Coverage Status

coverage: 30.197%. remained the same
when pulling d1a727c on zproc_workflow
into bd7fd13 on main.

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.

Thanks, as stated there are other existing ways to create N>1 desi_zproc batch scripts, but it's useful to be able to do it directly from desi_zproc --batch.

I have one comment inline to simplify an exit.

py/desispec/scripts/zproc.py Outdated Show resolved Hide resolved
@akremin
Copy link
Member

akremin commented Oct 11, 2024

Thanks for the quick turnaround. Once the checks are finished, I'll merge.

@akremin akremin merged commit fa2a2d8 into main Oct 11, 2024
26 checks passed
@akremin akremin deleted the zproc_workflow branch October 11, 2024 20:36
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