-
Notifications
You must be signed in to change notification settings - Fork 0
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
Pulling kbmod-tno-scripts over to the corresponding tasks. #18
Conversation
def no_uri_to_ic(target_uris_file_path=None, uris_base_dir=None, ic_output_file_path=None, logger=None): | ||
def uri_to_ic( | ||
uris_filepath: str = None, | ||
uris_base_dir: str = None, |
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.
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 I recall correctly, this was added because Colin was running the scripts both on klone and on another cluster where he had copied that target uris over (I believe on Monsoon). So he could then run the same script on the same target uris files but then specify a different base directory (the folder where he had copied the data to)
I don't think it's essential to keep in, but maybe not remove it until we settle whether we will be changing the .lst
files after Dino's work
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.
@ColinOrionChandler could you confirm this? Was this parameter in place so that you could run on multiple clusters?
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.
Thanks Drew! Left a few comments for some additional docstrings, but overall this looks correct to me.
While there are still items to clean up, I'm also fine with merging as we are so we can begin testing the current workflow on klone and see how parsl fares with our actual workloads before fiddling further.
def no_uri_to_ic(target_uris_file_path=None, uris_base_dir=None, ic_output_file_path=None, logger=None): | ||
def uri_to_ic( | ||
uris_filepath: str = None, | ||
uris_base_dir: str = None, |
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 I recall correctly, this was added because Colin was running the scripts both on klone and on another cluster where he had copied that target uris over (I believe on Monsoon). So he could then run the same script on the same target uris files but then specify a different base directory (the folder where he had copied the data to)
I don't think it's essential to keep in, but maybe not remove it until we settle whether we will be changing the .lst
files after Dino's work
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.
Thanks LGTM!
This PR is composed of several rough commits to pull over the code, including:
ic_to_wu
script.