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

fix(SingleTask): pull tag from input_tags, when not explicitly configured #170

Merged
merged 2 commits into from
Jan 24, 2022

Conversation

anjakefala
Copy link
Contributor

Fixes bug introduced in bbbc6b4. Input
tags stopped being referenced when setting the tag on the output.

Needed to tag delayspectrum files properly in the daily pipeline.

This reverts to previous behaviour. If this was behaviour that was intentionally removed, we will need to ensure that the tag is properly set on delayspectrum output containers.

…ured

Fixes bug introduced in bbbc6b4. Input
tags stopped being referenced when setting the tag on the output.

Needed to tag delayspectrum files properly in the daily pipeline.
@anjakefala anjakefala requested a review from jrs65 January 20, 2022 05:43
@jrs65
Copy link
Contributor

jrs65 commented Jan 24, 2022

There seems to be a few aspects to this one.

I was on the fence for a while about whether it should blindly copy the tag over from the input container. It previously did that, but it was all a bit of a hack as to how it worked, and so I had thought maybe it would be better for people to have to do it explicitly. But now I think I'm convinced now that it should do that, it's certainly better than the backup of just using the count as the tag.

The other aspect that confused me was why was this happening only in the DelaySpectrumEstimator. It looks like that's because that task does not use an attrs_from argument when constructing it's output container, which would have explicitly copied over the tag. Could you make that change, as it'll ensure both that tag is explicitly set, and that the other attributes are propagated?

Copy link
Contributor

@jrs65 jrs65 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, just a request to add the attrs_from argument (see my other comment).

@anjakefala
Copy link
Contributor Author

@jrs65 Made the requested change, and tested it without the input_tags change. delayspectrum files get tagged appropriately. =)

@anjakefala anjakefala requested a review from jrs65 January 24, 2022 21:25
@anjakefala anjakefala merged commit 12c4844 into master Jan 24, 2022
@anjakefala anjakefala deleted the fix-tag branch January 24, 2022 23:15
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