DM-39198: Ensure task connection storage classes are used in Quanta. #335
Conversation
| @@ -381,7 +399,10 @@ def __repr__(self) -> str: | |||
| quantum. | |||
| """ | |||
|
|
|||
| def makeQuantum(self, datastore_records: Optional[Mapping[str, DatastoreRecordData]] = None) -> Quantum: | |||
| def makeQuantum( | |||
There was a problem hiding this comment.
Why did this get reformatted? I can't see what changed. Was it reformatted because of an early API modification that was backed out?
There was a problem hiding this comment.
Yes, that's exactly it. Black vs. the trailing commas.
QuantumGraph should have the connection storage classes in its Quantum objects, and the registry storage classes for those dataset types in its "registryDatasetTypes" attribute. Apparently we were making no effort to ensure the former before.
TallJimbo
force-pushed
the
tickets/DM-39198
branch
from
5fce6de
to
0bb70e4
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #335 +/- ##
==========================================
- Coverage 82.65% 82.55% -0.10%
==========================================
Files 62 62
Lines 6960 6978 +18
Branches 1381 1388 +7
==========================================
+ Hits 5753 5761 +8
- Misses 926 935 +9
- Partials 281 282 +1
☔ View full report in Codecov by Sentry. |
|
|
||
| # Get data repository definitions from the QuantumGraph; these can have | ||
| # different storage classes than those in the quanta. | ||
| dataset_types = {dstype.name: dstype for dstype in qgraph.registryDatasetTypes()} |
There was a problem hiding this comment.
I see this line enough I'm starting to wonder if QuantumGraph could have a method that returned the dict.
There was a problem hiding this comment.
👍 . I'm informally saving up a number of changes I'd like to make to QG.
| for ref in original_output_refs: | ||
| internal_dataset_type = dataset_types.get(ref.datasetType.name, ref.datasetType) | ||
| if internal_dataset_type.storageClass_name != ref.datasetType.storageClass_name: | ||
| output_refs.add(ref.overrideStorageClass(internal_dataset_type.storageClass_name)) |
There was a problem hiding this comment.
| output_refs.add(ref.overrideStorageClass(internal_dataset_type.storageClass_name)) | |
| output_refs.add(ref.overrideStorageClass(internal_dataset_type.storageClass)) |
since the storage class should be attached and can be used directly without having to proxy through the name string.
There was a problem hiding this comment.
If you think that's safe, sure. I tend to default to using the name when I'm not sure because I don't want to accidentally trigger imports of things that might not be available, but that may just be a habit relevant for working down in the guts of registry, not here.
There was a problem hiding this comment.
I think at this point the storage class is going to be resolved into a real storage class almost immediately so it won't matter. I guess there's no harm in going via the name here and then deferring sotrage class creation as much as possible.
QuantumGraph should have the connection storage classes in its Quantum objects, and the registry storage classes for those dataset types in its "registryDatasetTypes" attribute. Apparently we were making no effort to ensure the former before.
Checklist
doc/changes