DM-42306: Performance optimizations #942

timj · 2024-01-16T21:26:08Z

Checklist

ran Jenkins
added a release note for user-visible changes to doc/changes

codecov · 2024-01-16T21:36:05Z

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (b7211c8) 88.37% compared to head (f516a08) 88.39%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
python/lsst/daf/butler/_config.py	80.00%	1 Missing and 1 partial ⚠️
python/lsst/daf/butler/_config_support.py	50.00%	1 Missing and 1 partial ⚠️
python/lsst/daf/butler/mapping_factory.py	77.77%	1 Missing and 1 partial ⚠️
python/lsst/daf/butler/_location.py	95.23%	0 Missing and 1 partial ⚠️
python/lsst/daf/butler/datastore/file_templates.py	97.77%	0 Missing and 1 partial ⚠️
python/lsst/daf/butler/datastores/fileDatastore.py	93.75%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #942      +/-   ##
==========================================
+ Coverage   88.37%   88.39%   +0.01%     
==========================================
  Files         303      303              
  Lines       38979    39070      +91     
  Branches     8221     8238      +17     
==========================================
+ Hits        34448    34535      +87     
- Misses       3339     3341       +2     
- Partials     1192     1194       +2

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

python/lsst/daf/butler/datastores/fileDatastore.py

TallJimbo

Optimizations look great.

I'm not sure I understand how the transfer-from dry-run option works (maybe because I'm not sure precisely how the checking for existing datasets in the target butler works even without dry-run).

I do think we should drop the ThreadPoolExecutor commit for now.

python/lsst/daf/butler/_butler.py

python/lsst/daf/butler/datastores/fileDatastore.py

python/lsst/daf/butler/datastore/file_templates.py

python/lsst/daf/butler/_location.py

If the relative path is coming directly from a datastore record we know it can't be something that is trying to ../ out of the datastore. The safety check in the Location constructor has significant overhead. Although with client/server this might be less true.

This can lead to some small performance improvement when ResourcePath does not need to check itself whether something is a file or not.

This makes it easier for code to copy a Location without having to know if deepcopy should be used.

The .fields method was slow and also being called four times. Now call a grouped fields method once that returns all the answers.

Add __deepcopy__ and __copy__ special methods that use clone()

Important now that we disable check for this in Location constructor.

This reverts commit f212bf5. We have decided that we are not read for the thread pool speedup.

timj · 2024-01-22T22:39:41Z

I'm not sure I understand how the transfer-from dry-run option works (maybe because I'm not sure precisely how the checking for existing datasets in the target butler works even without dry-run).

It does all the work that it would have to do to do the transfer but it doesn't copy any files or write to the target butler. I just modified how it works such that it will report that N datasets have already been transferred rather than dry run assuming that it always has to transfer everything.

Dry run is more useful if it really does report what it is going to do rather than reporting that it might transfer all the files to the target butler.

No longer need to check for None.

timj mentioned this pull request Jan 16, 2024

DM-42306: Support --dry-run flag in transfer-from-graph lsst/pipe_base#399

Merged

2 tasks

timj force-pushed the tickets/DM-42306 branch from 61c476b to 3c93daa Compare January 16, 2024 21:28

timj force-pushed the tickets/DM-42306 branch 10 times, most recently from b16c8b9 to 81b8fc3 Compare January 17, 2024 23:36

timj requested a review from TallJimbo January 18, 2024 15:59

timj commented Jan 18, 2024

View reviewed changes

python/lsst/daf/butler/datastores/fileDatastore.py Outdated Show resolved Hide resolved

timj force-pushed the tickets/DM-42306 branch from 9a69807 to a6d2fd2 Compare January 18, 2024 16:40

TallJimbo approved these changes Jan 18, 2024

View reviewed changes

timj added 13 commits January 22, 2024 11:53

Refresh pre-commit

4755690

Add a dry_run parameter to Butler.transfer_from

d002cb9

Avoid retrieving the dimension value twice in loop

f185c6a

Only ask for records that the template needs

660180d

Convert the lookup content to dict from Config

7c742bf

Only merge if there is something to merge

3c00e60

Annotate some ResourcePath constructors that we know are files

45fc4fa

This can lead to some small performance improvement when ResourcePath does not need to check itself whether something is a file or not.

Add Location.clone() method

e0bc065

This makes it easier for code to copy a Location without having to know if deepcopy should be used.

Now force LocationFactory to be using files in datastore

4c16b5e

Speed up template validation

72b679b

The .fields method was slow and also being called four times. Now call a grouped fields method once that returns all the answers.

We know that adding butler.yaml is a file URI

0a242dd

Tweak the exception raised when bad directory is used as file

50f4856

timj added 8 commits January 22, 2024 11:53

Ensure that Location.clone() minimizes copying

16490ed

Add __deepcopy__ and __copy__ special methods that use clone()

Add news fragments

001b788

Fix handling of skypix in file template

7879a4f

Use collections.abc rather than typing

a50d620

Remove extraneous parens (UP034)

9c7f390

Use type instead of typing.Type (UP006)

090b56b

Add check for ../ in file template

e129ba9

Important now that we disable check for this in Location constructor.

Experimental: Thread pool executor

f212bf5

timj force-pushed the tickets/DM-42306 branch from a6d2fd2 to f7253d4 Compare January 22, 2024 18:53

Revert "Experimental: Thread pool executor"

c243fd7

This reverts commit f212bf5. We have decided that we are not read for the thread pool speedup.

timj force-pushed the tickets/DM-42306 branch from cd73e09 to f2dfdfc Compare January 22, 2024 22:24

timj force-pushed the tickets/DM-42306 branch 2 times, most recently from 5138c62 to cc7c1cb Compare January 23, 2024 16:07

timj added 2 commits January 23, 2024 09:17

Fix updatedFile usage to be more explicit

9b842e9

Allow dry-run to report when it has already transferred something

8ee6309

Dry run is more useful if it really does report what it is going to do rather than reporting that it might transfer all the files to the target butler.

timj force-pushed the tickets/DM-42306 branch 2 times, most recently from 4d5ec6a to f1578f7 Compare January 23, 2024 16:21

timj added 2 commits January 23, 2024 09:32

Get the dimension values directly from the data ID

4d4ecce

No longer need to check for None.

Unhide trusted_path parameter

f516a08

timj force-pushed the tickets/DM-42306 branch 2 times, most recently from cc7657e to f516a08 Compare January 23, 2024 23:06

timj merged commit 458dcd7 into main Jan 23, 2024
18 checks passed

timj deleted the tickets/DM-42306 branch January 23, 2024 23:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-42306: Performance optimizations #942

DM-42306: Performance optimizations #942

timj commented Jan 16, 2024 •

edited

codecov bot commented Jan 16, 2024 •

edited

TallJimbo left a comment

timj commented Jan 22, 2024

DM-42306: Performance optimizations #942

DM-42306: Performance optimizations #942

Conversation

timj commented Jan 16, 2024 • edited

Checklist

codecov bot commented Jan 16, 2024 • edited

Codecov Report

TallJimbo left a comment

Choose a reason for hiding this comment

timj commented Jan 22, 2024

timj commented Jan 16, 2024 •

edited

codecov bot commented Jan 16, 2024 •

edited