DM-40441: first batch of deprecations from RFC-949 #411
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #411 +/- ##
==========================================
- Coverage 82.40% 80.88% -1.52%
==========================================
Files 93 92 -1
Lines 10660 10685 +25
Branches 2020 2049 +29
==========================================
- Hits 8784 8643 -141
- Misses 1522 1699 +177
+ Partials 354 343 -11 ☔ View full report in Codecov by Sentry. |
TallJimbo
force-pushed
the
tickets/DM-40441
branch
2 times, most recently
from
defaef5
to
692638f
Compare
This was added _just_ before TaskDef was approved for deprecation on RFC-949, and I'm sure nothing outside ctrl_mpexec is using it, so I'm not going through a deprecation period for it.
After DM-38498 datastore records were mistakenly being attached whether requested or not.
Apparently this was only exported publicly by the graphBuilder module, and while that was sort of an accident (it wasn't in __all__), it also makes sense as the only code using it, and that makes all_dimensions_quantum_graph_builder the logical new home.
TallJimbo
force-pushed
the
tickets/DM-40441
branch
from
74eec4f
to
842e592
Compare
TallJimbo
force-pushed
the
tickets/DM-40441
branch
from
d10eabe
to
4c8f616
Compare
2 tasks
timj
approved these changes
Apr 5, 2024
| @@ -101,6 +102,19 @@ class QuantumNode: | |||
| creation. | |||
| """ | |||
|
|
|||
| @property | |||
| def task_node(self) -> TaskNode: | |||
There was a problem hiding this comment.
Is this property something that can easily be added to a test?
| @@ -44,13 +48,31 @@ | |||
| if TYPE_CHECKING: | |||
| from .taskFactory import TaskFactory | |||
|
|
|||
| # TODO: remove this module on DM-40443. | |||
| warnings.warn( | |||
There was a problem hiding this comment.
Doesn't look like anything in lsst_distrib or ci is using this code. GitHub shows no-one using it either (other than forks of pipe_base) so removing this file completely seems entirely possible.
There was a problem hiding this comment.
Given that I've already got the deprecation in, I figure it'll still be easier for me to leave it as is and drop it on DM-40443.
| @@ -601,6 +596,17 @@ def findSubsetsWithLabel(self, label: str) -> set[str]: | |||
| results.add(subset.label) | |||
| return results | |||
|
|
|||
| @property | |||
| def task_labels(self) -> Set[str]: | |||
| @@ -260,6 +261,47 @@ def __deepcopy__(self, memo: dict) -> PipelineGraph: | |||
| # that mutable state is copied. | |||
| return self.copy() | |||
|
|
|||
| def diff_tasks(self, other: PipelineGraph) -> list[str]: | |||
There was a problem hiding this comment.
Is it possible to test this in pipe_base?
Eventually (DM-40442) these classes will be more tightly integrated, but this piece lets us start to modernize downstream code in advance.
Like the pipeline_graph attribute of QuantumGraph, eventually (DM-40442) we'll want this to be more tightly integrated, probably replacing the TaskDef as the true attribute, but for now this is enough to allow downstream code to be more forward-looking by avoiding deprecated or soon-to-be-deprecated TaskDef interfaces.
Passing a subclass instance of Task.ConfigClass is a weird thing to do, but it's seems harder to prohibit it than just accept it, and we have some tests in ctrl_mpexec that depend on doing just that.
This also makes the first argument to TaskFactory.makeTask positional-only, which is formally backwards incompatible but not something I'm worried about in practice.
I originally hoped we could minimize how much of the PipelineTaskConnections interface PipelineGraph would have to expose, with the idea being that PipelineTaskConnections would be one interface that task authors write to, and TaskNode would be a slightly different interface that the execution system writes against, and that decoupling those would be good for stability. But we've always had to punch holes to allow the task to customize its execution in various respects, and at this point it's best to just provide access to the connections class itself, too, even if we'd still prefer to have other TaskNode interfaces used by the execution system where possible.
This was the last usage of BaseConnections.makeDatasetType, so we can deprecate it now.
This also solidifies the definition of what we mean by label-range slicing for task that lack a dependency relationship to one or both bounds.
FieldValidationError has more useful state for diagonstics than most other exceptions, and that's worth preserving when validation fails.
v26 came and went while this branch was sidetracked.
This provides a way to get the labels of the tasks without the expensive (and not-always-possible) step of making a PipelineGraph.
TallJimbo
force-pushed
the
tickets/DM-40441
branch
from
218f1a1
to
b4cf9cf
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
doc/changes