DM-19988: support code (and removal of dead code) for QuantumGraph generation rewrite #169
Conversation
config/schema.yaml
Outdated
| primary_key: true | ||
| nullable: false | ||
| doc: > | ||
| Unique (with instrument) integer identifier for an visit. |
| The Run this Quantum is a part of. | ||
| initInputs : collection of `DatasetRef` |
| **kwargs): | ||
| super().__init__(**kwargs) | ||
| if taskClass is not None: | ||
| taskName = f"{taskClass.__module__}.{taskClass.__name__}" |
There was a problem hiding this comment.
Everyone seem to prefer f-strings for simple string operations, I still like old fashioned
taskClass.__module__ + "." + taskClass.__name__
There was a problem hiding this comment.
Yeah, I do like the f-string version better. In this case there isn't much of a difference, but I like using the same pattern for simple and more complex cases that seem to be part of the same family of patterns, and I think this qualifies.
| if initInputs is None: | ||
| initInputs = {} | ||
| elif not hasattr(initInputs, "keys"): | ||
| initInputs = {ref.datasetType: ref for ref in initInputs} |
There was a problem hiding this comment.
Could it be that there is more than one ref in initInputs with the same dataset type?
There was a problem hiding this comment.
No, the PipelineTask declarations already assume all initInput and initOutput dataset types are scalar (and they have to be because they have empty data IDs).
| self._initInputs = NamedKeyDict(initInputs) | ||
| self._predictedInputs = NamedKeyDict(predictedInputs if predictedInputs is not None else {}) | ||
| self._actualInputs = NamedKeyDict(actualInputs if actualInputs is not None else {}) | ||
| self._outputs = NamedKeyDict(outputs if outputs is not None else {}) |
There was a problem hiding this comment.
Some serious typing could be saved if NamedKeyDict constructor accepted None. Or can't you use old pattern outputs or {}?
There was a problem hiding this comment.
I don't think None makes sense for NamedKeyDict in general, so I'd prefer not to move the handling there. And I once asked about a variant of the outputs or {} pattern on Slack/#software-def, and it was not popular (people considered it a Perl pattern that was a Python antipattern). But I've come up with another solution: it's safe to pass an empty tuple NamedKeyDict's constructor, so I'll just use that as the default for these arguments.
MultipleDatasetQueryBuilder and its Registry-level interface were highly specialized for QuantumGraph generation but didn't actually do what we need. DataIdQueryBuilder is simpler, lower-level and hopefully more generally useful. The higher-level logic for QuantumGraph generation is being moved to pipe_base, which will now use DataIdQueryBuilder (and SingleDatasetQueryBuilder) directly.
Eventually we may want to add this to the database representation as well, but that's a bit tricky due to the variable number of dimensions, and if we partition the dataset table in the future that may provide a model.
These weren't really thought through and are not currently in use, so right now they're just a maintenance burden.
TallJimbo
force-pushed
the
tickets/DM-19988
branch
2 times, most recently
from
6db6cfb
to
eaeeae2
Compare
|
|
||
| Parameters | ||
| ---------- | ||
| sqlExpression | ||
| SQLAlchemy boolean column expression. | ||
| expression : str |
In particular, do not begin transactions before read-only operations, and use nested transactions on write operations. This seems to be necessary in order to do a Datastore read inside a QueryBuilder result-iteration loop.
TallJimbo
force-pushed
the
tickets/DM-19988
branch
from
ed50c93
to
d14b9c9
Compare
No description provided.