[23.1] Fix metadata setting in extended metadata + outputs_to_working_directory mode #16678
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
```
outputs = [
Bunch(false_path=os.path.join(outputs_directory, os.path.basename(path)), real_path=path)
for path in self.get_output_files(job_wrapper)
]
```
would always set the false_path attribute, even if that is not correct
because outputs_to_working_directory isn't activated and pulsar doesn't
need to do any staging (e.g. with
```
default_file_action: copy
# don't copy outputs, not needed.
file_actions:
paths:
- path_types: output
action: none
```
as done in embedded_pulsar_metadata_extended_job_conf.xml.
Fixing this allows us to eliminate the `if object_store:` guard to
setting the external filename, which in turn fixes the metadata value
setting if `outputs_to_working_directory: true` and `metadata_strategy:
extended` is used.
We'll surely need that too, as well as a datatypes config file. I'd assume this probably didn't fully work before and that we have no tests ? It does seem a little hard to test and I found no traces of tests.
|
Wow, cool! |
jmchilton
approved these changes
Sep 12, 2023
|
Alright, so now the extended + outputs to working dir tests are broken ... |
This looks quite weird, can we just get the outputs from the tool run maybe ?
mvdbeek
force-pushed
the
fix_metadata_setting
branch
from
3b16a93 to
01d4d43
Compare
mvdbeek
force-pushed
the
fix_metadata_setting
branch
from
cddc58b to
3930c6a
Compare
|
Hmm, I'm not quite sure what's up with the converter tests ... it seems other PRs are passing this test, and locally this passes too (but I can only test with |
|
OK, the |
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.
would always set the false_path attribute, even if that is not correct
because outputs_to_working_directory isn't activated and pulsar doesn't
need to do any staging (e.g. with
as done in embedded_pulsar_metadata_extended_job_conf.xml.
Fixing this allows us to eliminate the
if object_store:guard tosetting the external filename, which in turn fixes the metadata value
setting if
outputs_to_working_directory: trueandmetadata_strategy: extendedis used.We didn't catch this previously because of the
outpus_to_working_dirtypo ...The actual, isolated fix is in 8c5ac57, but I noticed that
use_metadata_binaryis probably broken in pulsar, so 6210267 should also fix that.How to test the changes?
(Select all options that apply)
License