Manifest Reproduce true fix #475
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previous behaviour would only use the exe path and restarts already set in the Manifest. So they would pick up when the exe/restart that were currently pointing toin the manifest would change, but not the configured one.
… when files are added to manifest - When reproduce is True, set scanInputs to True so new/changed inputs are discovered - When reproduce is True, only create work symlinks when filepaths are added in manifest during model.setup - Check for files that exist in the manifest, but haven't been added to work directory, or no longer exist
|
Hello @jo-basevi! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ok so I've had a go changing the manifests reproduce True logic. Currently (on the main branch), it create symlinks to the work directory from the manifests, and only raise errors if the files that existing on the manifest are modified.
In the first commit, I changed logic in model setup to always add model exe path and prior restart path to the manifest. This picks up if exe path is changed in
config.yaml. If there's new restart files, in the prior restart path, it will also pick it up that there's been a change.It still had the logic that symlinks from files to the work directory are generated from the manifest initially when reproduce is True. This could run into weird issues when there's a file in the manifest, thats existing unchanged on the filesystem, but it's no longer added to manifest in
Model.setup. It'll pass the manifest checks because the hashes are unchanged and this file will still be linked into the work directory.In the second commit, I have changed it to when reproduce is True, all the symlinks are created from filepaths added in the
model.setup()when it callsmanifest.add_filepath(). I have used the pre-existingexisting_filepathsset on thePayuManifestclass to keep track of what files haven't been added to the work directory but are in the manifest. There is now a check at the end ofPayuManifest.fastcheckchecking if that set is empty when reproduce is True.The second commit, also has changes to the
input/scanInputslogic. I've changed it to havescaninputsTrue whenreproduceis True. This is so changes to inputs inconfig.yamlare picked up, or if new files are added to input directories.scaninputscan be set to False, whenreproduceis False. This is will still not check for changes inconfig.yamlas it'll create symlinks only from the input manifest. If a file in the manifest no longer exists, it will still be removed from the manifests before manifests are checked (only when reproduce isFalse).I'll appreciate any feedback on these changes as it is changing the behaviour of reproduce:True quite a bit, and I'm unsure of my assumptions..
Closes #456