[24.0] Do not copy purged outputs to object store #18342
Merged
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
mvdbeek
force-pushed
the
fix_copying_purged_files
branch
from
ed022ba to
6be29de
Compare
This in particular needs a lot of new tests. We will also need to actively purge datasets in the model store import code, since users might have purged datasets while the job ran. Again, more tests needed.
mvdbeek
force-pushed
the
fix_copying_purged_files
branch
from
6be29de to
8e6e25d
Compare
jmchilton
reviewed
Jun 7, 2024
| # Users can purge outputs before the job completes, | ||
| # in that case we don't want to copy the output to a purged path. | ||
| # Static, non work_dir_output files are handled in job_finish code. | ||
| return f'\nif [ -f "{source_file}" -a -f "{destination}" ] ; then cp "{source_file}" "{destination}" ; fi' |
There was a problem hiding this comment.
This seems to be a wild check 😭. I hate it but I don't know the alternative. I know people love the disk object store and jobs writing directly to the object store 🤔.
There was a problem hiding this comment.
It's a bit of an "optimization" .. I'm happy to restore the original version, the important thing is that we don't store data in purged datasets.
These are only written if the command actually ran, so we'd fail here for instance if the outputs are deleted before the job had a chance to run.
mvdbeek
force-pushed
the
fix_copying_purged_files
branch
from
0467e96 to
94bd0c5
Compare
Add a sleep so we can delay output purging until command line is templated. If we don't sleep we generate a command line where the output path is just `''`. That needs another fix!
mvdbeek
force-pushed
the
fix_copying_purged_files
branch
from
94bd0c5 to
03eca7c
Compare
This is a tricky one. There could be systematic issues here that an admin would want to fix, but it could also just be the case that a user forced a wrong datatype. Ideally we'd probably tag this as job execution issue ? It's not very different from a job error IMO.
jmchilton
approved these changes
Jun 10, 2024
|
I haven't tested extra_files handling yet, and I know that command line templating is broken if you purge the output dataset before the job is run and the command line is queued if you're not using But there's so many fixes in here and they have coverage, so let's get this in (if the tests pass) while I work on the remaining tests. |
mvdbeek
force-pushed
the
fix_copying_purged_files
branch
from
25bca7c to
f0e09cc
Compare
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.
Fixes #18337. Needs a whole lot of tests still, this is just a rough pass at the most obvious cases where this might happen.
Tests to write:
extra_files* dynamic outputs * extended_metadataHow to test the changes?
(Select all options that apply)
License