Page MenuHomeSoftware Heritage
Differential D5349

package.loader: Lookup packages from the ExtID storage
ClosedPublic
Actions

Authored by vlorentz on Mar 25 2021, 7:22 PM.

Details

Reviewers
ardumont
Group Reviewers
Reviewers
Maniphest Tasks
T3140: Make loaders use ExtId to find what artifacts are already loaded, in addition to revision.metadata
Commits
rDLDBASEa32f68715102: package.loader: Lookup packages from the ExtID storage
Summary

To check which packages are already downloaded.

For now, this lookup is done in addition to checking the artifacts
from the last snapshot's revisions' metadata, because we did not start
writing ExtIDs yet.
But the ExtID lookup will eventually replace the artifact-based lookup.

This will finally allow us to drop the 'metadata' field of Revision
objects.

Depends on D5348

Resolves T3140

Diff Detail

Event Timeline

vlorentz created this revision.Mar 25 2021, 7:22 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptMar 25 2021, 7:22 PM
swh-public-ci added a comment.Mar 25 2021, 7:30 PM

Build is green

Patch application report for D5349 (id=19161)

Could not rebase; Attempt merge onto 6d3545e47b...

Updating 6d3545e..effa790
Fast-forward
 .pre-commit-config.yaml                          |   1 +
 swh/loader/package/debian/loader.py              |   4 +-
 swh/loader/package/debian/tests/test_debian.py   |  36 ++--
 swh/loader/package/loader.py                     | 184 +++++++++++++----
 swh/loader/package/nixguix/tests/test_nixguix.py |   6 +-
 swh/loader/package/tests/test_loader.py          | 247 +++++++++++++++++++++--
 6 files changed, 409 insertions(+), 69 deletions(-)
Changes applied before test
commit effa79036ef92ebba38fd6907292f79085701a2f
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Mar 25 19:17:17 2021 +0100

    package.loader: Lookup packages from the ExtID storage
    
    To check which packages are already downloaded.
    
    For now, this lookup is done in addition to checking the artifacts
    from the last snapshot's revisions' metadata, because we did not start
    writing ExtIDs yet.
    But the ExtID lookup will eventually replace the artifact-based lookup.
    
    This will finally allow us to drop the 'metadata' field of Revision
    objects.

commit 9ad94ddc3d99f9d702ae6411e0723c0080548c82
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Mar 25 16:19:45 2021 +0100

    test_resolve_revision_from_artifacts: Use the right type for PartialExtID.
    
    We used a string instead of a tuple. It doesn't matter much because they
    are only compared with each other, but let's not intentionally use
    the wrong types when we don't need to.

commit 6e04a21df73e3d208b50159e78b52b1d6443ea83
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Mar 25 14:52:37 2021 +0100

    test_loader: Use a spec'ed mock, to follow the new style guide

commit 2f9b8a8b1eecb578cdcbe9046262b6087a6c15de
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Mar 25 10:47:03 2021 +0100

    Rename resolve_revision_from with _artifacts suffix, and fix its documentation
    
    A future commit will introduce resolve_revision_from_extid, so this commit
    preemptively renames it to avoid any confusion.

commit 78430078cd2647e09cc9a0c09a9e6179d4a0d8b1
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Mar 23 16:14:42 2021 +0100

    package.loader: Unnest loops in PackageLoader.load()
    
    In a future commit, we will need to go through all the PackageInfo
    objects before running the loop, so we can get their ExtID and
    fetch them from the storage.
    
    So, we need to fetch them all before running the load loop,
    using this listcomp.

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/443/ for more details.

Harbormaster completed remote builds in B20243: Diff 19161.Mar 25 2021, 7:30 PM
vlorentz requested review of this revision.Mar 25 2021, 7:30 PM
ardumont added a subscriber: ardumont.Mar 25 2021, 7:49 PM

Resolves T3140

Note that it's only true for the package loaders...
We also need to do it for other (d)vcs loaders (mercurial, svn, git)
prior to resolving the task.

vlorentz updated this revision to Diff 19168.Mar 26 2021, 11:18 AM

rebase (undo D5347)

ardumont added a comment.Mar 26 2021, 11:22 AM

Did not read entirely the tests yet... some remarks inline.

swh/loader/package/loader.py
293–296
356
368

Log this one as warning (or whatever is the right logging level).

swh-public-ci added a comment.Mar 26 2021, 11:22 AM

Build is green

Patch application report for D5349 (id=19168)

Could not rebase; Attempt merge onto 2f9b8a8b1e...

Updating 2f9b8a8..2ab982d
Fast-forward
 .pre-commit-config.yaml                 |   1 +
 swh/loader/package/loader.py            | 118 +++++++++++++++-
 swh/loader/package/tests/test_loader.py | 232 ++++++++++++++++++++++++++++++--
 3 files changed, 338 insertions(+), 13 deletions(-)
Changes applied before test
commit 2ab982dbbc669eee8a4969e53fe80d4fe4678ce6
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Mar 25 19:17:17 2021 +0100

    package.loader: Lookup packages from the ExtID storage
    
    To check which packages are already downloaded.
    
    For now, this lookup is done in addition to checking the artifacts
    from the last snapshot's revisions' metadata, because we did not start
    writing ExtIDs yet.
    But the ExtID lookup will eventually replace the artifact-based lookup.
    
    This will finally allow us to drop the 'metadata' field of Revision
    objects.

commit e590b4258262e78449d84724e25824af07b96c47
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Mar 25 16:19:45 2021 +0100

    test_resolve_revision_from_artifacts: Use the right type for PartialExtID.
    
    We used a string instead of a tuple. It doesn't matter much because they
    are only compared with each other, but let's not intentionally use
    the wrong types when we don't need to.

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/445/ for more details.

Harbormaster completed remote builds in B20249: Diff 19168.Mar 26 2021, 11:22 AM
vlorentz updated this revision to Diff 19172.Mar 26 2021, 11:28 AM

typos + logging

vlorentz updated this revision to Diff 19173.Mar 26 2021, 11:29 AM

another typo

swh-public-ci added a comment.Mar 26 2021, 11:30 AM

Build is green

Patch application report for D5349 (id=19172)

Rebasing onto e590b42582...

Current branch diff-target is up to date.
Changes applied before test
commit 0898142247e3ab5ef2d1a85994f96bc11c05b28e
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Mar 25 19:17:17 2021 +0100

    package.loader: Lookup packages from the ExtID storage
    
    To check which packages are already downloaded.
    
    For now, this lookup is done in addition to checking the artifacts
    from the last snapshot's revisions' metadata, because we did not start
    writing ExtIDs yet.
    But the ExtID lookup will eventually replace the artifact-based lookup.
    
    This will finally allow us to drop the 'metadata' field of Revision
    objects.

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/446/ for more details.

Harbormaster completed remote builds in B20253: Diff 19172.Mar 26 2021, 11:30 AM
swh-public-ci added a comment.Mar 26 2021, 11:32 AM

Build is green

Patch application report for D5349 (id=19173)

Rebasing onto e590b42582...

Current branch diff-target is up to date.
Changes applied before test
commit 0ca0b0df46cc7c3de29c8563882d52cdef88edf7
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Mar 25 19:17:17 2021 +0100

    package.loader: Lookup packages from the ExtID storage
    
    To check which packages are already downloaded.
    
    For now, this lookup is done in addition to checking the artifacts
    from the last snapshot's revisions' metadata, because we did not start
    writing ExtIDs yet.
    But the ExtID lookup will eventually replace the artifact-based lookup.
    
    This will finally allow us to drop the 'metadata' field of Revision
    objects.

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/447/ for more details.

Harbormaster completed remote builds in B20254: Diff 19173.Mar 26 2021, 11:32 AM
ardumont added inline comments.Mar 26 2021, 11:32 AM
swh/loader/package/tests/test_loader.py
141

you kept that pattern here :)

161

ok, re-read the code indeed, you whitelist only the revisions from the previous snapshot.

Is that only revision that are targetted by snapshot though?

nvm, you said so earlier "for now, yes".

ardumont accepted this revision.Mar 26 2021, 11:40 AM

Resolves T3140

Note that it's only true for the package loaders...
We also need to do it for other (d)vcs loaders (mercurial, svn, git)
prior to resolving the task.

Have you noticed my first remark about ^?

Nonetheless, lgtm.

I would not resolve the task just yet though (or rename the task to specifiically address the package loaders).

This revision is now accepted and ready to land.Mar 26 2021, 11:40 AM
vlorentz marked 3 inline comments as done.Mar 26 2021, 11:41 AM
vlorentz added inline comments.
swh/loader/package/tests/test_loader.py
161

what?

vlorentz updated this revision to Diff 19174.Mar 26 2021, 11:43 AM

remove return_extid pattern

ardumont added inline comments.Mar 26 2021, 11:44 AM
swh/loader/package/tests/test_loader.py
161
# We only support revisions for now.
# Note that this case should never be reached unless there is a
# collision between a revision hash and some non-revision object's
# hash, but better safe than sorry.
vlorentz added a comment.Mar 26 2021, 11:44 AM
In D5349#135985, @ardumont wrote:

Have you noticed my first remark about ^?

yes

This revision was landed with ongoing or failed builds.Mar 26 2021, 11:44 AM
Closed by commit rDLDBASEa32f68715102: package.loader: Lookup packages from the ExtID storage (authored by vlorentz). · Explain Why
This revision was automatically updated to reflect the committed changes.
vlorentz added a commit: rDLDBASEa32f68715102: package.loader: Lookup packages from the ExtID storage.
swh-public-ci added a comment.Mar 26 2021, 11:45 AM

Build is green

Patch application report for D5349 (id=19174)

Rebasing onto e590b42582...

Current branch diff-target is up to date.
Changes applied before test
commit a32f687151025289568fd0f410cf93df6cb304e0
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Mar 25 19:17:17 2021 +0100

    package.loader: Lookup packages from the ExtID storage
    
    To check which packages are already downloaded.
    
    For now, this lookup is done in addition to checking the artifacts
    from the last snapshot's revisions' metadata, because we did not start
    writing ExtIDs yet.
    But the ExtID lookup will eventually replace the artifact-based lookup.
    
    This will finally allow us to drop the 'metadata' field of Revision
    objects.

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/448/ for more details.

Harbormaster completed remote builds in B20255: Diff 19174.Mar 26 2021, 11:45 AM

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
1 line--
swh/
loader/
package/
118 lines97%
Loading...
tests/
212 lines99%
Loading...

Diff 19168

View Options

.pre-commit-config.yaml

Loading...
View Options

swh/loader/package/loader.py

Loading...
View Options

swh/loader/package/tests/test_loader.py

Loading...
About · Developer information · Legal