DM-41962: Use storage classes from QG in PreExecInit. #276
Conversation
PipelineDatasetTypes does preserve storage classes (that's why it's being deprecated).
TallJimbo
force-pushed
the
tickets/DM-41962
branch
from
9ccb217
to
dd2f147
Compare
First problem was just that the test assumed it could register a dataset type after the QG was made and not run into trouble when running that QG. Second problem was that it was just expecting something other than what its own code comments suggested, which was also contrary to correct behavior.
TallJimbo
force-pushed
the
tickets/DM-41962
branch
from
179fe1f
to
ae2ae85
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #276 +/- ##
==========================================
- Coverage 87.15% 87.05% -0.11%
==========================================
Files 49 49
Lines 4429 4433 +4
Branches 764 766 +2
==========================================
- Hits 3860 3859 -1
- Misses 413 421 +8
+ Partials 156 153 -3 ☔ View full report in Codecov by Sentry. |
| @@ -243,14 +237,21 @@ def test_from_pipeline_output_differ(self): | |||
| ) | |||
| ) | |||
|
|
|||
| executor = self._configure_pipeline( | |||
There was a problem hiding this comment.
This has to move until after the dataset type has been registered? If that is the case shouldn't the other tests have this change?
There was a problem hiding this comment.
Good point. They probably don't matter, but they're at least unsafe. Will do.
| # b returns a dict and that is converted to TaskMetadata on put. | ||
| self._test_logs(cm.output, "dict", "lsst.pipe.base.TaskMetadata", "dict", "dict") | ||
|
|
||
| self.assertEqual(len(quanta), 2) | ||
| self.assertEqual(self.butler.get("intermediate"), {"zero": 0, "one": 1}) | ||
| self.assertEqual(self.butler.get("intermediate").to_dict(), {"zero": 0, "one": 1}) | ||
| self.assertEqual(self.butler.get("output").to_dict(), {"zero": 0, "one": 1, "two": 2}) |
There was a problem hiding this comment.
So "output" is a TaskMetadata? How is that consistent with "b returned a dict" test above and the default connections being "dict"?
There was a problem hiding this comment.
From the perspective of b, it returned a dict, and declared that it would return a dict, but since the dataset type was registered with the repo as TaskMetadataLike it was converted to TaskMetadataLike on put.
There was a problem hiding this comment.
Ok. It should likely use the more explicit TaskMetadata.from_dict() in the test to make it obvious.
This includes: - Using TaskMetadata.from_dict instead of to_dict in comparisons to make the intent clearer. - Documenting what the log-inspection utility method actually tests. - Making one test (test_from_pipeline_intermediates_differ) registering a dataset type before building a QG because that's what's needed in general for correctness (even though it didn't matter here). - Renaming and re-documenting another test (test_from_pipeline_inconsistent_dataset_types) where we were actually testing that building the QG and then changing a dataset type out from under it can be a problem.
TallJimbo
force-pushed
the
tickets/DM-41962
branch
from
c1d1bfd
to
1a05997
Compare
|
I've added another commit that cleans up the test a bit; ready for another look. |
We have a custom QG builders in the wild and they're not as well-behaved as the main one; guard against them until we can fix them.
PipelineDatasetTypes does not preserve storage classes (that's why it's being deprecated).
Checklist
mainis broken in at least two ways right now, and this ticket fixes one of them)doc/changes