Details

Reviewers

vlorentz

Group Reviewers

Reviewers

Maniphest Tasks

T4595: Improve common tooling for loading

Commits

rDLDBASE1facea3cd215: Setup async interface for discovery module

Summary

This will allow us to use this interface in async code like `swh-scanner`.

Unfortunately, this means calling `asyncio.run` for sync code, but the
performance impact should be negligible.

The `swh_storage.*missing*` APIs are inconsistent for each type, which
requires a lot of boilerplate code. This should be addressed in a
follow-up.

There is also one hack (marked as such inline) needed to have an API that
isn't also plagued by implementation details of needing to keep
ID -> object matchers around. This should be a removed as a direct
consequence of aligning the storage APIs.

Diff Detail

Repository

rDLDBASE Generic VCS/Package Loader

Branch

discovery-D8538

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 31825
Build 49804: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 49803: arc lint + arc unit

Event Timeline

Alphare created this revision.Sep 26 2022, 6:29 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptSep 26 2022, 6:29 PM

Build is green

Patch application report for D8538 (id=30790)

Rebasing onto 798f749e66...

Current branch diff-target is up to date.

Changes applied before test

commit 19db5769cbe618b77c322189b66994222e2c8668
Author: Raphaël Gomès <rgomes@octobus.net>
Date:   Mon Sep 26 18:21:54 2022 +0200

    Setup async interface for discovery module
    
    This will allow us to use this interface in async code like ``swh-scanner``.
    
    Unfortunately, this means calling ``asyncio.run`` for sync code, but the
    performance impact should be negligible.
    
    The ``swh_storage.*missing*`` APIs are inconsistent for each type, which
    requires a lot of boilerplate code. This should be addressed in a
    follow-up.
    
    There is also one hack (marked as such inline) needed to have an API that
    isn't also plagued by implementation details of needing to keep
    ID -> object matchers around. This should be a removed as a direct
    consequence of aligning the storage APIs.

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

Harbormaster completed remote builds in B31767: Diff 30790.Sep 26 2022, 6:33 PM

Alphare requested review of this revision.Sep 26 2022, 6:33 PM

One question for reviewers: I've tried using Protocol for ArchiveDiscoveryInterface (should I?), but I'm not sure if it's possible to define a concrete method within the protocol, namely if __init__ could be implemented by default in ArchiveDiscoveryInterface.

Alphare added a child revision: D8539: Add random directory sampling policy.Sep 26 2022, 6:52 PM

classes should not inherit from protocols. Use an abstract class instead of a protocol if you want to use inheritance.

Use ABC

Build is green

Patch application report for D8538 (id=30795)

Rebasing onto 798f749e66...

Current branch diff-target is up to date.

Changes applied before test

commit fe6610629b1bff545b30ea86743d8e90a0487a47
Author: Raphaël Gomès <rgomes@octobus.net>
Date:   Mon Sep 26 18:21:54 2022 +0200

    Setup async interface for discovery module
    
    This will allow us to use this interface in async code like ``swh-scanner``.
    
    Unfortunately, this means calling ``asyncio.run`` for sync code, but the
    performance impact should be negligible.
    
    The ``swh_storage.*missing*`` APIs are inconsistent for each type, which
    requires a lot of boilerplate code. This should be addressed in a
    follow-up.
    
    There is also one hack (marked as such inline) needed to have an API that
    isn't also plagued by implementation details of needing to keep
    ID -> object matchers around. This should be a removed as a direct
    consequence of aligning the storage APIs.

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

Harbormaster completed remote builds in B31772: Diff 30795.Sep 26 2022, 11:00 PM

I don't understand why inconsistencies in swh-storage's API require passing this dict around.

Could you explain what needs to change in the swh-storage API, and how this code would look like after the change?

swh/loader/core/discovery.py
44–53	You need to decorate them with `@abc.abstractmethod`

vlorentz added inline comments.Sep 27 2022, 10:38 AM

swh/loader/core/discovery.py
29	docstring, please

Add docstring, abstract method decorators, explain hack

Build is green

Patch application report for D8538 (id=30851)

Rebasing onto 798f749e66...

Current branch diff-target is up to date.

Changes applied before test

commit 404c1b9a30928f42a8f32bbfbcfe64986fdc8126
Author: Raphaël Gomès <rgomes@octobus.net>
Date:   Mon Sep 26 18:21:54 2022 +0200

    Setup async interface for discovery module
    
    This will allow us to use this interface in async code like ``swh-scanner``.
    
    Unfortunately, this means calling ``asyncio.run`` for sync code, but the
    performance impact should be negligible.
    
    The ``swh_storage.*missing*`` APIs are inconsistent for each type, which
    requires a lot of boilerplate code. This should be addressed in a
    follow-up.
    
    There is also one hack (marked as such inline) needed to have an API that
    isn't also plagued by implementation details of needing to keep
    ID -> object matchers around. This should be a removed as a direct
    consequence of aligning the storage APIs. See inline comment.

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

Harbormaster completed remote builds in B31825: Diff 30851.Sep 27 2022, 4:56 PM

I still don't understand why you need the _all_contents hack instead of doing this:

async def skipped_content_missing(
    self, skipped_contents: List[Sha1Git]
) -> Iterable[Sha1Git]:
    """List skipped content missing from the archive by sha1"""
    contents = [{"sha1_git": s} s in skipped_contents]
    return (d["sha1_git"] for d in self.storage.skipped_content_missing(contents))

In D8538#222541, @vlorentz wrote:

I still don't understand why you need the _all_contents hack instead of doing this:

async def skipped_content_missing(
    self, skipped_contents: List[Sha1Git]
) -> Iterable[Sha1Git]:
    """List skipped content missing from the archive by sha1"""
    contents = [{"sha1_git": s} s in skipped_contents]
    return (d["sha1_git"] for d in self.storage.skipped_content_missing(contents))

Ah. I was sure skipped_content_missing took SkippedContent instead of dicts. Sorry about that, will update this patch soon.

Remove ugly hack

Build is green

Patch application report for D8538 (id=30912)

Rebasing onto 798f749e66...

Current branch diff-target is up to date.

Changes applied before test

commit 1facea3cd215155f77ae4083a33837b7c6f642b0
Author: Raphaël Gomès <rgomes@octobus.net>
Date:   Mon Sep 26 18:21:54 2022 +0200

    Setup async interface for discovery module
    
    This will allow us to use this interface in async code like ``swh-scanner``.
    
    Unfortunately, this means calling ``asyncio.run`` for sync code, but the
    performance impact should be negligible.
    
    The ``swh_storage.*missing*`` APIs are inconsistent for each type, which
    requires a lot of boilerplate code. This should be addressed in a
    follow-up.

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

Harbormaster completed remote builds in B31885: Diff 30912.Sep 28 2022, 5:43 PM

vlorentz accepted this revision.Sep 28 2022, 5:51 PM

This revision is now accepted and ready to land.Sep 28 2022, 5:51 PM

Closed by commit rDLDBASE1facea3cd215: Setup async interface for discovery module (authored by Alphare). · Explain WhySep 28 2022, 5:54 PM

This revision was automatically updated to reflect the committed changes.

Alphare added a commit: rDLDBASE1facea3cd215: Setup async interface for discovery module.

I'm coming late to the battle ¯\_(ツ)_/¯.

Still a couple of remarks/questions.

Isn't this missing an asyncio dependency requirements in /requirements.txt

Why is this defined here and not in the storage since all the current dicsovery methods involve the storage?

ardumont mentioned this in D8539: Add random directory sampling policy.Sep 28 2022, 7:14 PM

Still a couple of remarks/questions.

Isn't this missing an asyncio dependency requirements in /requirements.txt

nope, it's fine [1]

[1]

10:35 <+ardumont> is asyncio part of python3.7?
10:36 <+ardumont> (for loader.core and the new discovery async code ^)
10:36 <Alphare> yes
10:36 <Alphare> https://docs.python.org/3.7/library/asyncio.html

Why is this defined here and not in the storage since all the current dicsovery methods involve the storage?

see [2]

[2] https://forge.softwareheritage.org/D8539?id=30916#inline-60749

bchauvet added a task: T4595: Improve common tooling for loading.Sep 30 2022, 9:46 AM

Setup async interface for discovery module
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Patch application report for D8538 (id=30790)

Changes applied before test

Patch application report for D8538 (id=30795)

Changes applied before test

Patch application report for D8538 (id=30851)

Changes applied before test

Patch application report for D8538 (id=30912)

Changes applied before test

Revision Contents
Changeset List

Diff 30851

swh/loader/core/discovery.py

swh/loader/package/loader.py

Setup async interface for discovery moduleClosedPublicActions

Details

Diff Detail

Event Timeline

Patch application report for D8538 (id=30790)

Changes applied before test

Patch application report for D8538 (id=30795)

Changes applied before test

Patch application report for D8538 (id=30851)

Changes applied before test

Patch application report for D8538 (id=30912)

Changes applied before test

Revision ContentsChangeset List

Diff 30851

swh/loader/core/discovery.py

swh/loader/package/loader.py

Setup async interface for discovery module
ClosedPublic
Actions

Revision Contents
Changeset List