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

job submission: don't error out if standard templates missing; change comment in transitioner #2133

Merged
merged 3 commits into from
Sep 23, 2017

Conversation

davidpanderson
Copy link
Contributor

If you specify the input or output templates,
the standard ones (appname_in, appname_out) don't need to exist.

Also, in create_work():

  • don't read the output template; just check that it exists.
  • deprecate the result_template_filepath arg; redundant

If you specify the input or output templates,
the standard ones (appname_in, appname_out) don't need to exist.

Also, in create_work():
- don't read the output template; just check that it exists.
- deprecate the result_template_filepath arg; redundant
The name defaults to appname_PID_unixtime.
nanoHUB wanted to specify their own name to simplify associating
output files with HUB jobs.
@ChristianBeer
Copy link
Member

I don't really like that the changes to create_work() are hidden in the commit message. IMHO they are independent from the remote submission changes and warrant their own commit so the commit subject is more descriptive.

@davidpanderson
Copy link
Contributor Author

create_work is part of remote job submission.

@ChristianBeer
Copy link
Member

create_work is a standalone program that can be used for local job submission as well as remote job submission. You are changing the function which affects both use cases. My point is that someone looking for a change made to the standalone program doesn't recognize that a commit message titled "remote job submission" is affecting create_work too.

What explanation do you have for the comment change to the transitioner? This one is certainly not part of remote job submission.

In my eyes 921dcd1 is not atomic and contains unrelated changes.

@davidpanderson davidpanderson changed the title remote job submission: don't error out if standard templates missing job submission: don't error out if standard templates missing; change comment in transitioner Sep 21, 2017
@davidpanderson
Copy link
Contributor Author

I changed the title. Any problem w/ the content of the PR?

@ChristianBeer
Copy link
Member

Changing the title of the PR doesn't change the commit message where the unrelated change is embedded in. This is not what is meant by atomic commits. I stand by my objection which is about atomicity not about the quality of the code (which I think is fine).

@davidpanderson davidpanderson merged commit 8495e2f into master Sep 23, 2017
@davidpanderson davidpanderson deleted the dpa_template branch September 28, 2017 04:30
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.

2 participants