DM-39100 Move PipelineTask Config override handling #329
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #329 +/- ##
==========================================
+ Coverage 82.12% 82.13% +0.01%
==========================================
Files 60 60
Lines 6718 6723 +5
Branches 1374 1373 -1
==========================================
+ Hits 5517 5522 +5
Misses 923 923
Partials 278 278
☔ View full report in Codecov by Sentry. |
natelust
force-pushed
the
tickets/DM-39100
branch
3 times, most recently
from
a11ef83
to
63d2f00
Compare
python/lsst/pipe/base/config.py
Outdated
| from numbers import Number | ||
| from typing import TYPE_CHECKING, Any, Dict, Optional, Tuple, Type, TypeVar | ||
| from typing import TYPE_CHECKING, Any, Dict, Iterable, Optional, Tuple, Type, TypeVar |
There was a problem hiding this comment.
| from typing import TYPE_CHECKING, Any, Dict, Iterable, Optional, Tuple, Type, TypeVar | |
| from collections.abc import Iterable | |
| from typing import TYPE_CHECKING, Any, Dict, Optional, Tuple, Type, TypeVar |
You don't need to all the way in terms of modernizing type annotations here, but this much should already be consistent with the rest of the module and only involves the new symbol you've imported.
python/lsst/pipe/base/config.py
Outdated
| --------- | ||
| instrument : `Instrument` or `None` | ||
| If this is not None, this is an `Instrument` instance which | ||
| will be used to apply instrumental overrides. |
There was a problem hiding this comment.
I think we should just say that this was the Instrument instance referenced by the pipeline, or None if there was no instrument in the pipeline; it's now up to the task to decide whether to apply overrides with it (even if we expect them to almost always apply those overrides).
python/lsst/pipe/base/config.py
Outdated
| will be used to apply instrumental overrides. | ||
| taskDefaultName : `str` | ||
| The default name associated with the `Task` class. This is | ||
| used with instrumental overrides. |
There was a problem hiding this comment.
In keeping with previous comment, maybe say, "This may be used for instrumental overrides."
python/lsst/pipe/base/config.py
Outdated
| The default name associated with the `Task` class. This is | ||
| used with instrumental overrides. | ||
| pipelineConfigs : `Iterable` of `ConfigIR` | ||
| This is an iterable of `ConfigIR` objects that contain |
There was a problem hiding this comment.
| This is an iterable of `ConfigIR` objects that contain | |
| An iterable of `ConfigIR` objects that contain |
| if subConfig.python is not None: | ||
| overrides.addPythonOverride(subConfig.python) | ||
| for key, value in subConfig.rest.items(): | ||
| overrides.addValueOverride(key, value) |
There was a problem hiding this comment.
I'm guessing this is unchanged, but I would not have expected these overrides to be applied after the python: block when both exist, though I don't have a solid reason for that expectation.
There was a problem hiding this comment.
Value overrides are always last, as python block have the most side effects, importing things, retargeting, etc
| instrument: str | ||
| A string containing the fully qualified name of an instrument from | ||
| which configs should be loaded and applied | ||
| instrument: Instrument |
There was a problem hiding this comment.
| instrument: Instrument | |
| instrument : `Instrument` |
natelust
force-pushed
the
tickets/DM-39100
branch
from
63d2f00
to
6bbb89b
Compare
Move the handling of PipelineTaskConfig override handling into the Config class and out of the Pipeline. This allows Config classes more control over how overrides are applied, and a place to add a hook to react to config values.
natelust
force-pushed
the
tickets/DM-39100
branch
from
6bbb89b
to
abecf51
Compare
Remove the constrain that parameters must be defined in Pipelines, freeing up task config objects to make use of the info as they see fit.
natelust
force-pushed
the
tickets/DM-39100
branch
2 times, most recently
from
432892e
to
ada6f02
Compare
natelust
force-pushed
the
tickets/DM-39100
branch
from
ada6f02
to
9858a02
Compare
Move the handling of PipelineTaskConfig override handling into the Config class and out of the Pipeline. This allows Config classes more control over how overrides are applied, and a place to add a hook to react to config values.
Checklist
doc/changes