DM-36591: Manually handle component datasets that appear as outputs. #287
Conversation
Components probably shouldn't be appearing at all here, and that will be investigated on DM-33027. For now we apply the same logic used for input datasets to handle components manually, sincer DM-36312 has deprecated support for components in registry queries.
TallJimbo
force-pushed
the
tickets/DM-36591
branch
from
a45292d
to
9a9dd61
Compare
Codecov ReportBase: 81.82% // Head: 81.78% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #287 +/- ##
==========================================
- Coverage 81.82% 81.78% -0.04%
==========================================
Files 56 56
Lines 6150 6155 +5
Branches 1127 1128 +1
==========================================
+ Hits 5032 5034 +2
- Misses 886 888 +2
- Partials 232 233 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| component = datasetType.component() | ||
| else: | ||
| parent_dataset_type = datasetType | ||
| component = None |
There was a problem hiding this comment.
I'm a bit nervous about datasetType, parent_dataset_type, and component coexisting here, though I think the only questionable reference to the first is new line 943. (It doesn't help that datasetType is also used for loop variables which overwrite this one, though -- please consider factoring this massive method at some point!)
There was a problem hiding this comment.
I think leaving datasetType in line 943 is best - it's slightly more informative than parent_dataset_type in that error message. And I agree that this method should get refactored at some point, and some good opportunities are coming up in tickets that are already on my near-term to-do list.
| ) | ||
| except MissingDatasetTypeError: | ||
| resolvedRefQueryResults = [] | ||
| for resolvedRef in resolvedRefQueryResults: | ||
| assert resolvedRef.dataId in refs | ||
| refs[resolvedRef.dataId] = resolvedRef | ||
| refs[resolvedRef.dataId] = ( | ||
| resolvedRef.makeComponentRef(component) if component is not None else resolvedRef |
There was a problem hiding this comment.
Code duplication; I suspect these multiple loops over resolvedRefQueryResults can be combined.
There was a problem hiding this comment.
Agreed, and I noticed the same; I'm just trying to make minimal changes to get this in the weekly with the smallest chance of disruption. DM-33027 is going to overhaul all of this, and that's pretty high up my priority list.
|
Thanks. I'm probably going to defer any further changes for now, provided my ongoing Jenkins run succeeds, as I'm not sure I have time to make changes and run it again before the weekly is tagged. But I will get those fixed in the not-too-distant future. As for the coverage gap, we've got a lot of graph-building coverage further down in ctrl_mpexec that we should move up to pipe_base, but in this particular case I'm pretty sure there's no coverage there or even in ci_hsc, ci_imsim, or ci_cpp, because my previous effort to make this warning an error on a branch didn't trip any of those. Until about an hour ago I was pretty mystified as to what's going on and didn't have any idea how to write test for it, but some pair-coding work with @leeskelvin on drp_pipe unit tests gave me a hint that I think I'll be able to follow up on DM-33027 to make sure the better fix for this is well-tested in some package, even if it isn't here. |
|
After all that talk about trying to get this in the weekly, I just crashed after getting the kids to bed last night and completely forgot to follow up and merge in time. I'm still just going to merge this as-is, as I think the issues on this PR predate it and are better handled more thoroughly later, but apologies for having to see the warnings for another week. |
Components probably shouldn't be appearing at all here, and that will be investigated on DM-33027. For now we apply the same logic used for input datasets to handle components manually, sincer DM-36312 has deprecated support for components in registry queries.
Checklist
doc/changes