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.

Sign up for GitHub

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

Jump to bottom

DM-38614: more fixes for storage class conversion support #316

Merged
merged 2 commits into from
Apr 12, 2023
Merged

DM-38614: more fixes for storage class conversion support #316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f47b2a3
Be more careful (yet again) about storage classes in execution butler.
TallJimbo Apr 7, 2023
c4b68be
Improve logging about storage class conversions in QG gen.
TallJimbo Apr 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/changes/DM-38614.bugfix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a bug that led to valid storage class conversions being rejected when using execution butler.
69 changes: 37 additions & 32 deletions python/lsst/pipe/base/executionButlerBuilder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@
from collections import defaultdict
from typing import Callable, DefaultDict, Iterable, List, Mapping, Optional, Set, Tuple, Union

from lsst.daf.butler import Butler, Config, DataCoordinate, DatasetRef, DatasetType
from lsst.daf.butler import Butler, Config, DataCoordinate, DatasetRef, DatasetType, Registry
from lsst.daf.butler.core.repoRelocation import BUTLER_ROOT_TAG
from lsst.daf.butler.registry import ConflictingDefinitionError
from lsst.daf.butler.registry import ConflictingDefinitionError, MissingDatasetTypeError
from lsst.daf.butler.transfers import RepoExportContext
from lsst.resources import ResourcePath, ResourcePathExpression
from lsst.utils.introspection import get_class_of
Expand All @@ -41,7 +41,7 @@


def _validate_dataset_type(
candidate: DatasetType, previous: dict[Union[str, DatasetType], DatasetType]
candidate: DatasetType, previous: dict[Union[str, DatasetType], DatasetType], registry: Registry
) -> DatasetType:
"""Check the dataset types and return a consistent variant if there are
different compatible options.
Expand All @@ -54,6 +54,9 @@
Previous dataset types found, indexed by name and also by
dataset type. The latter provides a quick way of returning a
previously checked dataset type.
registry : `lsst.daf.butler.Registry`
Main registry whose dataset type registration should override the
given one if it exists.

Returns
-------
Expand Down Expand Up @@ -99,8 +102,16 @@
f"Dataset type incompatibility in graph: {prevDsType} not compatible with {candidate}"
)

# New dataset type encountered. Store it by name and by dataset type
# so it will be validated immediately next time it comes up.
# We haven't seen this dataset type in this graph before, but it may
# already be in the registry.
try:
registryDsType = registry.getDatasetType(name)
previous[candidate] = registryDsType
return registryDsType
except MissingDatasetTypeError:
pass

Check warning on line 112 in python/lsst/pipe/base/executionButlerBuilder.py

View check run for this annotation

Codecov / codecov/patch

python/lsst/pipe/base/executionButlerBuilder.py#L107-L112 

Added lines #L107 - L112 were not covered by tests
# Dataset type is totally new. Store it by name and by dataset type so
# it will be validated immediately next time it comes up.
previous[name] = candidate
previous[candidate] = candidate
return candidate
Expand Down Expand Up @@ -136,7 +147,7 @@
# the dataset IDs, so we process them there even though each Quantum for a
# task has the same ones.
for dataset_type in itertools.chain(dataset_types.initIntermediates, dataset_types.initOutputs):
dataset_type = _validate_dataset_type(dataset_type, datasetTypes)
dataset_type = _validate_dataset_type(dataset_type, datasetTypes, butler.registry)

Check warning on line 150 in python/lsst/pipe/base/executionButlerBuilder.py

View check run for this annotation

Codecov / codecov/patch

python/lsst/pipe/base/executionButlerBuilder.py#L150 

Added line #L150 was not covered by tests
inserts[dataset_type].add(DataCoordinate.makeEmpty(dataset_type.dimensions.universe))

# Output references may be resolved even if they do not exist. Find all
Expand Down Expand Up @@ -178,14 +189,22 @@
# exported.
if ref.isComponent():
ref = ref.makeCompositeRef()
# Make sure we export this with the registry's dataset
# type, since transfer_from doesn't handle storage
# class differences (maybe it should, but it's not
# bad to be defensive here even if that changes).
type = _validate_dataset_type(type, datasetTypes, butler.registry)

Check warning on line 196 in python/lsst/pipe/base/executionButlerBuilder.py

View check run for this annotation

Codecov / codecov/patch

python/lsst/pipe/base/executionButlerBuilder.py#L196 

Added line #L196 was not covered by tests
if type != ref.datasetType:
ref = ref.overrideStorageClass(type.storageClass)
assert ref.datasetType == type, "Dataset types should not differ in other ways."

Check warning on line 199 in python/lsst/pipe/base/executionButlerBuilder.py

View check run for this annotation

Codecov / codecov/patch

python/lsst/pipe/base/executionButlerBuilder.py#L198-L199 

Added lines #L198 - L199 were not covered by tests
exports.add(ref)
else:
if ref.isComponent():
# We can't insert a component, and a component will
# be part of some other upstream dataset, so it
# should be safe to skip them here
continue
type = _validate_dataset_type(type, datasetTypes)
type = _validate_dataset_type(type, datasetTypes, butler.registry)

Check warning on line 207 in python/lsst/pipe/base/executionButlerBuilder.py

View check run for this annotation

Codecov / codecov/patch

python/lsst/pipe/base/executionButlerBuilder.py#L207 

Added line #L207 was not covered by tests
inserts[type].add(ref.dataId)
return exports, inserts

Expand All @@ -208,13 +227,12 @@
return collections


def _export(
butler: Butler, collections: Optional[Iterable[str]], exports: Set[DatasetRef], inserts: DataSetTypeMap
) -> io.StringIO:
# This exports the datasets that exist in the input butler using
# daf butler objects, however it reaches in deep and does not use the
# public methods so that it can export it to a string buffer and skip
# disk access.
def _export(butler: Butler, collections: Optional[Iterable[str]], inserts: DataSetTypeMap) -> io.StringIO:
# This exports relevant dimension records and collections using daf butler
# objects, however it reaches in deep and does not use the public methods
# so that it can export it to a string buffer and skip disk access. This
# does not export the datasets themselves, since we use transfer_from for
# that.
yamlBuffer = io.StringIO()
# Yaml is hard coded, since the class controls both ends of the
# export/import
Expand Down Expand Up @@ -336,23 +354,10 @@

# Register datasets to be produced and insert them into the registry
for dsType, dataIds in inserts.items():
# There may be inconsistencies with storage class definitions
# so those differences must be checked.
try:
newButler.registry.registerDatasetType(dsType)
except ConflictingDefinitionError:
# We do not at this point know whether the dataset type is
# an intermediate (and so must be able to support conversion
# from the registry storage class to an input) or solely an output
# dataset type. Test both compatibilities.
registryDsType = newButler.registry.getDatasetType(dsType.name)
if registryDsType.is_compatible_with(dsType) and dsType.is_compatible_with(registryDsType):
# Ensure that we use the registry type when inserting.
dsType = registryDsType
else:
# Not compatible so re-raise the original exception.
raise

# Storage class differences should have already been resolved by calls
# _validate_dataset_type in _export, resulting in the Registry dataset
# type whenever that exists.
newButler.registry.registerDatasetType(dsType)

Check warning on line 360 in python/lsst/pipe/base/executionButlerBuilder.py

View check run for this annotation

Codecov / codecov/patch

python/lsst/pipe/base/executionButlerBuilder.py#L360 

Added line #L360 was not covered by tests
newButler.registry.insertDatasets(dsType, dataIds, run)

return newButler
Expand Down Expand Up @@ -453,7 +458,7 @@
dataset_types = PipelineDatasetTypes.fromPipeline(graph.iterTaskGraph(), registry=butler.registry)

exports, inserts = _accumulate(butler, graph, dataset_types)
yamlBuffer = _export(butler, collections, exports, inserts)
yamlBuffer = _export(butler, collections, inserts)

Check warning on line 461 in python/lsst/pipe/base/executionButlerBuilder.py

View check run for this annotation

Codecov / codecov/patch

python/lsst/pipe/base/executionButlerBuilder.py#L461 

Added line #L461 was not covered by tests

newButler = _setupNewButler(butler, outputLocation, dirExists, datastoreRoot)

Expand Down
26 changes: 21 additions & 5 deletions python/lsst/pipe/base/graphBuilder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,27 @@
# stream code will be more explicit about input
# vs output incompatibilities.
existing = combined_by_name[datasetType.name]
if existing.is_compatible_with(datasetType) or datasetType.is_compatible_with(existing):
_LOG.warning(
"Dataset type mismatch (%s != %s) but continuing since they are compatible",
datasetType,
existing,
convertible_to_existing = existing.is_compatible_with(datasetType)
convertible_from_existing = datasetType.is_compatible_with(existing)

Check warning on line 164 in python/lsst/pipe/base/graphBuilder.py

View check run for this annotation

Codecov / codecov/patch

python/lsst/pipe/base/graphBuilder.py#L163-L164 

Added lines #L163 - L164 were not covered by tests
if convertible_to_existing and convertible_from_existing:
_LOG.debug(

Check warning on line 166 in python/lsst/pipe/base/graphBuilder.py

View check run for this annotation

Codecov / codecov/patch

python/lsst/pipe/base/graphBuilder.py#L166 

Added line #L166 was not covered by tests
"Dataset type %s has multiple fully-compatible storage classes %s and %s",
datasetType.name,
datasetType.storageClass_name,
existing.storageClass_name,
)
_dict[datasetType] = combined[existing]

Check warning on line 172 in python/lsst/pipe/base/graphBuilder.py

View check run for this annotation

Codecov / codecov/patch

python/lsst/pipe/base/graphBuilder.py#L172 

Added line #L172 was not covered by tests
elif convertible_to_existing or convertible_from_existing:
Copy link
Member

Choose a reason for hiding this comment

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

The code in the graph builder tries to work out whether this is solely an output or solely an input or is an intermediate and so doesn't always require that both directions work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you saying I should drop this message entirely because by the time we get here we've always checked things more thoroughly? I know we've got various checks in various places, but wasn't sure about the flow, so here I just wanted to preserve the original message while making it less scary.

Copy link
Member

Choose a reason for hiding this comment

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

I think I was mainly commenting on the comment saying that we can't tell whether this is an error or not. I'm not sure where in the graph builder this is occurring so it might be that the test that compare outputs to intermediates to inputs happen later. I do remember trying to be careful about only complaining (and failing to build the graph) if you try to use something as an intermediate that won't convert in both directions (it's possible that is buggy of course).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, ok, all I meant with the comment was that this code right here doesn't have the information it needs to make that determination. Other parts of the graph builder definitely can.

# We'd need to refactor a fair amount to recognize
# whether this is an error or not, so I'm not going to
# bother until we need to do that for other reasons
# (it won't be too long).
_LOG.info(

Check warning on line 178 in python/lsst/pipe/base/graphBuilder.py

View check run for this annotation

Codecov / codecov/patch

python/lsst/pipe/base/graphBuilder.py#L178 

Added line #L178 was not covered by tests
"Dataset type %s is present with multiple only partially-compatible storage "
"classes %s and %s.",
datasetType.name,
datasetType.storageClass_name,
existing.storageClass_name,
)
_dict[datasetType] = combined[existing]
else:
Expand Down