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

Added force option to setup and run #256

Merged
merged 12 commits into from
May 11, 2020
Merged

Added force option to setup and run #256

merged 12 commits into from
May 11, 2020

Conversation

aidanheerdegen
Copy link
Collaborator

Added force option to setup and run. Automatically removes existing work directory. Closes #255.

Also hanged default of reproduce option to False from None.

work directory.

Changed default of reproduce option to False from None.
@pep8speaks
Copy link

pep8speaks commented May 6, 2020

Hello @aidanheerdegen! 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 2020-05-08 09:26:20 UTC

@aidanheerdegen
Copy link
Collaborator Author

This isn't finished yet. Have to make a test. Also haven't made output overwrite. @marshallward do we want to add that too? I'm broadly in favour, just wanted to check.

@marshallward
Copy link
Collaborator

I don't know how output override would work, since if a run crashed then there ought not be an output directory. (If there were then I'd consider it a bug).

But if there's some problem I can't see that you think it will solve, then I reckon it's alright.

@aidanheerdegen
Copy link
Collaborator Author

Not really. The instructions say you can payu run -i n but don't mention you would have to remove the output00n directory to do this. Same with the reproduce flag. You can use it to re-run an experiment, but it won't work if there is an existing output directory.

@aidanheerdegen
Copy link
Collaborator Author

So it is solving a different problem.

@marshallward
Copy link
Collaborator

OK, then I could see how it could be useful.

I had never used the payu run -i n stuff much, but there ought to be better support for navigating the history, and wiping and re-running old jobs would certainly be a part of that.

@aidanheerdegen
Copy link
Collaborator Author

Maybe do that in a different PR I guess, rather than overload this one right now?

@marshallward
Copy link
Collaborator

Sounds good, smaller is better!

@aidanheerdegen
Copy link
Collaborator Author

Still have to add a test, which I'll do tomorrow as I am off for now. Thanks for the feedback @marshallward

@aidanheerdegen
Copy link
Collaborator Author

Well I went a bit mad and added a bunch of command line argument tests, which did show up a weird errant option that was not a bug but was not correct.

Pretty happy with the resulting tests. Not testing all commands, but easy enough to add them in if you feel so inclined.

@aidanheerdegen
Copy link
Collaborator Author

I reckon I'll just go ahead and merge this @marshallward

@aidanheerdegen aidanheerdegen merged commit 9ee82d3 into master May 11, 2020
@aidanheerdegen aidanheerdegen deleted the issue-255 branch January 23, 2024 05:08
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.

Add option to overwrite existing directories
3 participants