DM-46776: Add zip creation and ingest #1105

timj · 2024-10-22T20:06:41Z

Checklist

ran Jenkins
added a release note for user-visible changes to doc/changes
(if changing dimensions.yaml) make a copy of dimensions.yaml in configs/old_dimensions

codecov · 2024-10-22T20:21:26Z

Codecov Report

Attention: Patch coverage is 82.19178% with 104 lines in your changes missing coverage. Please review.

Project coverage is 89.37%. Comparing base (ef8b14b) to head (c510c7f).
Report is 40 commits behind head on main.

Files with missing lines	Patch %	Lines
...er/datastores/file_datastore/retrieve_artifacts.py	72.50%	27 Missing and 6 partials ⚠️
python/lsst/daf/butler/datastores/fileDatastore.py	83.33%	14 Missing and 8 partials ⚠️
python/lsst/daf/butler/_dataset_ref.py	86.15%	5 Missing and 4 partials ⚠️
...on/lsst/daf/butler/remote_butler/_remote_butler.py	70.96%	6 Missing and 3 partials ⚠️
python/lsst/daf/butler/cli/cmd/commands.py	52.94%	6 Missing and 2 partials ⚠️
...hon/lsst/daf/butler/datastores/chainedDatastore.py	84.61%	3 Missing and 3 partials ⚠️
...on/lsst/daf/butler/direct_butler/_direct_butler.py	90.69%	2 Missing and 2 partials ⚠️
python/lsst/daf/butler/formatters/json.py	33.33%	4 Missing ⚠️
python/lsst/daf/butler/formatters/yaml.py	42.85%	4 Missing ⚠️
python/lsst/daf/butler/script/ingest_zip.py	66.66%	2 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1105      +/-   ##
==========================================
- Coverage   89.46%   89.37%   -0.10%     
==========================================
  Files         361      362       +1     
  Lines       47787    48288     +501     
  Branches     5799     5872      +73     
==========================================
+ Hits        42752    43155     +403     
- Misses       3649     3718      +69     
- Partials     1386     1415      +29

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

andy-slac · 2024-10-24T21:02:54Z

@timj, ticket says that butler zip-from-graph should also be implemented, but I can't see it? I see that QBB adds retrieve_artifacts_zip, but I can't find anything that uses that method.

timj · 2024-10-24T21:04:21Z

@andy-slac it's in the pipe_base PR. lsst/pipe_base#452

andy-slac

Looks good, small bunch of comments/questions. I tried to understand as much as I could, but some pieces are very new to me (or I forgot much of it already).

python/lsst/daf/butler/_butler.py

python/lsst/daf/butler/_formatter.py

python/lsst/daf/butler/_quantum_backed.py

python/lsst/daf/butler/datastores/file_datastore/retrieve_artifacts.py

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

python/lsst/daf/butler/direct_butler/_direct_butler.py

python/lsst/daf/butler/remote_butler/_remote_butler.py

Fpr chained datastore retrieveArtifacts was checking each dataset for file existence in a loop which can be really slow on GCS. Instead use knows_these and assume the datastore does have the file if it has the ref.

Refactor some of the zip index code as part of this.

No longer use list[DatasetRef] but use SerializedDatasetRefContainer

This should be fine since we attempt to make the file names unique. It also works around an issue in ResourcePath where a "#" character is treated as part of a directory path if found in the non-file part of the path.

…path

For now rejects if any refs are rejected.

…rtifacts This allows Zip retrieval to work in QBB mode.

This makes it easier to work out which file comes from which dataset ref and guarantees each file will be unique without directory path.

This does not fix the problem of retrieve-artifacts retrieving Zip files and ending up with a bad index.

Co-authored-by: Andy Salnikov <[email protected]>

And remove some duplicated code that was writing the index a second time.

This allows pydantic to drop them from the JSON.

Which is rare with the new query system but is possible.

There is nothing to be gained by including all the dimensions other than taking extra space.

This makes it much more explicit what the parameters mean.

The mapping included all the paths so no reason to return the paths separately.

This is important to ensure that the same prefix is used each time if the artifact has multiple refs associated with it.

timj force-pushed the tickets/DM-46776 branch 4 times, most recently from fd8d31d to 4eecd6a Compare October 22, 2024 22:21

timj mentioned this pull request Oct 23, 2024

DM-46776: Add butler zip-from-graph command lsst/pipe_base#452

Merged

2 tasks

timj force-pushed the tickets/DM-46776 branch from c932f1c to 03874b4 Compare October 23, 2024 19:33

andy-slac approved these changes Oct 24, 2024

View reviewed changes

timj force-pushed the tickets/DM-46776 branch 6 times, most recently from 41d08e1 to 37090e1 Compare October 29, 2024 22:34

timj mentioned this pull request Oct 30, 2024

DM-46776: Add new ingest_zip method required to Datastore lsst/analysis_tools#303

Merged

timj force-pushed the tickets/DM-46776 branch from 6a2484d to 9449aea Compare October 30, 2024 20:35

timj added 12 commits October 30, 2024 13:40

Add Butler.retrieve_artifacts_zip()

6a4a62b

Trust that the datastore knows an artifact exists without checking

96d86d4

Fpr chained datastore retrieveArtifacts was checking each dataset for file existence in a loop which can be really slow on GCS. Instead use knows_these and assume the datastore does have the file if it has the ref.

Write index file with retrieveArtifacts and use it for Zip

2489f41

Refactor some of the zip index code as part of this.

Add --zip command-line option to butler retrieve-artifacts

cf25c13

Change the internal representation of ZipIndex to remove duplication

81c827d

No longer use list[DatasetRef] but use SerializedDatasetRefContainer

Add method to create ZipIndex from zip file

fafce2b

First attempt at butler ingest_zip

0bafc7a

Strip directory paths when building zip file.

1d6ce51

This should be fine since we attempt to make the file names unique. It also works around an issue in ResourcePath where a "#" character is treated as part of a directory path if found in the non-file part of the path.

Add butler ingest-zip command

642b90f

Do not remove Zip file until last dataset in Zip is removed

d19df2e

Fragments can be quoted so have to look for unquoted fragment in zip …

b078e7b

…path

Add minimal constraints checking for zip ingest

b165dab

For now rejects if any refs are rejected.

Add tests for ingest-zip

244c9b6

timj force-pushed the tickets/DM-46776 branch from 9449aea to 1b157d3 Compare October 30, 2024 20:41

timj and others added 25 commits October 30, 2024 13:47

Add support for missing datastore records in trust mode for retrieveA…

fd40aeb

…rtifacts This allows Zip retrieval to work in QBB mode.

Add UUID prefix to each file placed in Zip

a53db14

This makes it easier to work out which file comes from which dataset ref and guarantees each file will be unique without directory path.

Attempt to prevent repeated copying of the same file for DECam/Zips

05bc923

This does not fix the problem of retrieve-artifacts retrieving Zip files and ending up with a bad index.

Minor doc string fixes

9ef21fe

Co-authored-by: Andy Salnikov <[email protected]>

Minor tweaks from review

24c4701

Rename Zip index file name

d06f36a

And remove some duplicated code that was writing the index a second time.

Use partition rather than split

372d54c

Fix file datastore retrieve artifacts index for DECam data

16afb61

Provide defaults for checksum and component

8141248

This allows pydantic to drop them from the JSON.

Reorganize index to only have one dict for path in zip

0e8af26

Switch to sets just in case there are duplicates

4576a80

Which is rare with the new query system but is possible.

Remove a warning in test code and report warnings from caller

c8cb406

Support zip unpacking in retrieve artifacts

a57e725

Include version information in ZipIndex model

97c2d29

Only serialize required dimensions for dataset type and data coordinate

7db9443

There is nothing to be gained by including all the dimensions other than taking extra space.

Ensure that zip unpacking also respects overwrite flag

f2cbd09

Use a protocol for retrieve artifacts callback

516d714

This makes it much more explicit what the parameters mean.

Simplify Datastore.retrieveArtifacts return value

0c75a82

The mapping included all the paths so no reason to return the paths separately.

Support clobber/no-clobber when writing Zip archive

13b0f15

Raise rather than use assert

5082563

Add tests for reading V1 ZipIndex JSON model

82cf784

Sort refs when retrieving artifacts

b9ee95b

This is important to ensure that the same prefix is used each time if the artifact has multiple refs associated with it.

Enable compression of the index in the zip file

bb9bab4

Make SerializedDatasetRefContainerV1 public

8b8c250

Add news fragment

c510c7f

timj force-pushed the tickets/DM-46776 branch from 1b157d3 to c510c7f Compare October 30, 2024 20:47

timj merged commit 2b0d39d into main Oct 30, 2024
15 of 16 checks passed

timj deleted the tickets/DM-46776 branch October 30, 2024 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-46776: Add zip creation and ingest #1105

DM-46776: Add zip creation and ingest #1105

timj commented Oct 22, 2024 •

edited

Loading

codecov bot commented Oct 22, 2024 •

edited

Loading

andy-slac commented Oct 24, 2024

timj commented Oct 24, 2024

andy-slac left a comment

DM-46776: Add zip creation and ingest #1105

DM-46776: Add zip creation and ingest #1105

Conversation

timj commented Oct 22, 2024 • edited Loading

Checklist

codecov bot commented Oct 22, 2024 • edited Loading

Codecov Report

andy-slac commented Oct 24, 2024

timj commented Oct 24, 2024

andy-slac left a comment

Choose a reason for hiding this comment

timj commented Oct 22, 2024 •

edited

Loading

codecov bot commented Oct 22, 2024 •

edited

Loading