DM-40441: first batch of deprecations from RFC-949 #287

TallJimbo · 2024-03-28T21:01:51Z

Checklist

ran Jenkins
~~added a release note for user-visible changes to doc/changes~~ (the pipe_base changelog entry covers these changes, too)

The same code was already being called in the end, but now we're bypassing the soon-to-be-deprecated backwards-compatibility shim.

These have apparently been there from the beginning, but seem to have never been used and never documented.

This also deprecates iterable-of-TaskDef inputs to SimplePipelineExecutor.

This deprecates the 'builder' argument to make_quantum_graph in favor of the new 'builder_class' argument.

Since the QG is now frequently responsible for carrying information about registry dataset types to executing code, we don't want it to be legal to change the registry definition after making a QG and then run that QG. We also can't really stop it most of the time, but we can at least make these tests not do it.

codecov · 2024-03-28T21:06:29Z

Codecov Report

Attention: Patch coverage is 79.28571% with 58 lines in your changes are missing coverage. Please review.

Project coverage is 86.83%. Comparing base (716b37d) to head (f5ec60c).

❗ Current head f5ec60c differs from pull request most recent head fab8c00. Consider uploading reports for the commit fab8c00 to get more accurate results

Files	Patch %	Lines
python/lsst/ctrl/mpexec/singleQuantumExecutor.py	72.50%	10 Missing and 1 partial ⚠️
python/lsst/ctrl/mpexec/taskFactory.py	54.54%	8 Missing and 2 partials ⚠️
python/lsst/ctrl/mpexec/preExecInit.py	63.63%	6 Missing and 2 partials ⚠️
python/lsst/ctrl/mpexec/cmdLineFwk.py	64.70%	5 Missing and 1 partial ⚠️
...thon/lsst/ctrl/mpexec/separablePipelineExecutor.py	50.00%	4 Missing and 2 partials ⚠️
python/lsst/ctrl/mpexec/log_capture.py	69.23%	2 Missing and 2 partials ⚠️
...ython/lsst/ctrl/mpexec/simple_pipeline_executor.py	76.47%	4 Missing ⚠️
python/lsst/ctrl/mpexec/dotTools.py	25.00%	3 Missing ⚠️
tests/test_executors.py	93.02%	0 Missing and 3 partials ⚠️
python/lsst/ctrl/mpexec/util.py	86.66%	1 Missing and 1 partial ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #287      +/-   ##
==========================================
- Coverage   87.29%   86.83%   -0.47%     
==========================================
  Files          49       49              
  Lines        4472     4542      +70     
  Branches      771      780       +9     
==========================================
+ Hits         3904     3944      +40     
- Misses        415      441      +26     
- Partials      153      157       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

timj

My main question is about the v26 vs v27 timeline for removal.

python/lsst/ctrl/mpexec/log_capture.py

timj · 2024-04-05T17:42:03Z

python/lsst/ctrl/mpexec/preExecInit.py

-                    ):
-                        raise
+                    expected = self.full_butler.registry.getDatasetType(dataset_type.name)
+                    raise ConflictingDefinitionError(


Since you are raising the same exception can you instead annotate the original exception?

except ConflictingDefinitionError as e: expected = self.full_butler.registry.getDatasetType(dataset_type.name) e.add_note( f"DatasetType definition in registry has changed since the QuantumGraph was built: " f"{dataset_type} (graph) != {expected} (registry)." ) raise

?

I gave this a try, but that changes which message appears as the primary one, and in this case we want the new message to be more prominent than the original.

timj · 2024-04-05T17:46:57Z

python/lsst/ctrl/mpexec/preExecInit.py

                except ConflictingDefinitionError:
-                    if not _check_compatibility(


I assume the compatibility check is now done lower down and I haven't read the pipe_base changes yet.

Yes, specifically in PipelineGraph.resolve.

timj · 2024-04-05T17:50:18Z

python/lsst/ctrl/mpexec/separablePipelineExecutor.py

+        if builder:
+            warnings.warn(
+                "The 'builder' argument to SeparablePipelineBuilder.make_quantum_graph "
+                "is deprecated in favor of 'builder_class', and will be removed after v26.",


You want to remove it all before 27 comes out?

No, just an old branch I forgot to update.

timj · 2024-04-05T17:53:31Z

python/lsst/ctrl/mpexec/simple_pipeline_executor.py

-        quantum_graph = graph_builder.makeGraph(
-            pipeline, collections=butler.collections, run=butler.run, userQuery=where, bind=bind
+            # TODO: disable this block and adjust docs and annotations
+            # on DM-40443.


Maybe DM-40443 should be made a blocker of DM-37430? (the v27 pipelines release ticket)

Given the errors I made in version numbers on this ticket, I'm setting the release ticket to block DM-40443 instead.

timj · 2024-04-05T18:07:32Z

requirements.txt

@@ -5,7 +5,7 @@ pydantic >=2,<3.0
 networkx
 git+https://github.com/lsst/resources@main#egg=lsst-resources


Please modernize all these when you go in to remove the ticket branch.

lsst-resources @ git+https://github.com/lsst/resources@main lsst-daf-butler @ git+https://github.com/lsst/daf_butler@main lsst-utils @ git+https://github.com/lsst/utils@main lsst-pipe-base @ git+https://github.com/lsst/pipe_base@main lsst-pex-config @ git+https://github.com/lsst/pex_config@main

...instead of PipelineDatasetTypes. This includes a check on changes to registry dataset types between QG generation and execution, but only when executing with full butler.

Calling code in this package will be updated to avoid the new warning in a later commit.

This should eliminate all new deprecation warnings from internal ctrl_mpexec calls.

It's easier to deprecate the whole function than it is to migrate its return type.

I'm deferring actually removing this usage to DM-40639, since there's an opportunity to easily make a nice improvement to this code at the same time, but only after DM-39779 merges (and I don't want to increase the scope of this ticket further).

Co-authored-by: Tim Jenness <[email protected]>

TallJimbo added 8 commits March 28, 2024 11:25

Add VERBOSE logging around QG saving.

7274e9b

Switch to new QG generation interfaces in CmdLineFwk.

0473f1e

The same code was already being called in the end, but now we're bypassing the soon-to-be-deprecated backwards-compatibility shim.

Remove unused **kwargs from SimplePipelineExecutor.from_pipeline.

5a50b19

These have apparently been there from the beginning, but seem to have never been used and never documented.

Modernize SimplePipelineExecutor via PipelineGraph/QGB.

20d0a87

This also deprecates iterable-of-TaskDef inputs to SimplePipelineExecutor.

Use new QG builders in SeparablePipelineExecutor.

c3c0d63

This deprecates the 'builder' argument to make_quantum_graph in favor of the new 'builder_class' argument.

Drop usage of PipelineDatasetTypes in tests.

a299263

Drop usage of PipelineDatasetTypes in CmdLineFwk.

fd6236c

timj approved these changes Apr 5, 2024

View reviewed changes

TallJimbo force-pushed the tickets/DM-40441 branch from 2269c20 to 3c77a63 Compare April 8, 2024 16:05

TallJimbo added 8 commits April 8, 2024 12:28

Use PipelineGraph in PreExecInit.

c3d9f8c

...instead of PipelineDatasetTypes. This includes a check on changes to registry dataset types between QG generation and execution, but only when executing with full butler.

Support TaskNode and deprecate TaskDef in TaskFactory.

d8a2085

Calling code in this package will be updated to avoid the new warning in a later commit.

Support TaskNode and deprecate TaskDef in LogCapture.

a789093

Calling code in this package will be updated to avoid the new warning in a later commit.

Support TaskNode and deprecate TaskDef in executors.

42318dd

This should eliminate all new deprecation warnings from internal ctrl_mpexec calls.

Replace util.filterTasks with util.filterTaskNodes.

ec74e39

It's easier to deprecate the whole function than it is to migrate its return type.

Avoid toExpandedPipeline, Pipeline.__iter__ in ShowInfo and tests.

38f6a44

Silence deprecation warning in dotTools.

45a5595

I'm deferring actually removing this usage to DM-40639, since there's an opportunity to easily make a nice improvement to this code at the same time, but only after DM-39779 merges (and I don't want to increase the scope of this ticket further).

Handle missing dataset types for prerequisite inputs more carefully.

10e92fd

TallJimbo force-pushed the tickets/DM-40441 branch from 3c77a63 to 977d342 Compare April 8, 2024 16:28

TallJimbo and others added 2 commits April 8, 2024 12:28

Add 'deprecated' package type stub dependency.

6d58cc1

Bump deprecation->removal version from v26 to v27.

3f35ac0

Co-authored-by: Tim Jenness <[email protected]>

TallJimbo force-pushed the tickets/DM-40441 branch 2 times, most recently from 94a2200 to 734861c Compare April 8, 2024 16:37

Modernize requirements.txt.

fab8c00

TallJimbo force-pushed the tickets/DM-40441 branch from 734861c to fab8c00 Compare April 8, 2024 20:03

TallJimbo merged commit d04c916 into main Apr 8, 2024
13 checks passed

TallJimbo deleted the tickets/DM-40441 branch April 8, 2024 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-40441: first batch of deprecations from RFC-949 #287

DM-40441: first batch of deprecations from RFC-949 #287

TallJimbo commented Mar 28, 2024 •

edited by timj

Loading

codecov bot commented Mar 28, 2024 •

edited

Loading

timj left a comment

timj Apr 5, 2024

TallJimbo Apr 8, 2024

timj Apr 5, 2024

TallJimbo Apr 7, 2024

timj Apr 5, 2024

TallJimbo Apr 7, 2024

timj Apr 5, 2024

TallJimbo Apr 7, 2024

timj Apr 5, 2024

		except ConflictingDefinitionError:
		if not _check_compatibility(

		@@ -5,7 +5,7 @@ pydantic >=2,<3.0
		networkx
		git+https://github.com/lsst/resources@main#egg=lsst-resources

DM-40441: first batch of deprecations from RFC-949 #287

DM-40441: first batch of deprecations from RFC-949 #287

Conversation

TallJimbo commented Mar 28, 2024 • edited by timj Loading

Checklist

codecov bot commented Mar 28, 2024 • edited Loading

Codecov Report

timj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallJimbo commented Mar 28, 2024 •

edited by timj

Loading

codecov bot commented Mar 28, 2024 •

edited

Loading