Page MenuHomeSoftware Heritage
Differential D5071

Unify loader instantiation
ClosedPublic
Actions

Authored by ardumont on Feb 12 2021, 4:11 PM.

Details

Reviewers
vlorentz
olasd
Group Reviewers
Reviewers
Maniphest Tasks
T1410: Kill implicit configuration: new configuration scheme
Commits
rDLDBASE7116bb75897a: Unify loader instantiation
Summary

This unifies and centralizes the instantiation the same way the lister does.

This introduces a new base class swh.loader.core.loader.Loader for all loaders whose only
concern for now is to instantiate loaders from either a configuration dict or a
configuration file.

This simplify instantiation in celery task code and avoids duplicating the configuration
load in each loader constructor.

The end goal is to simplify the future refactoring on configuration. With the following,
we will only have to adapt the Loader class when we finally start simplifying uniformly
the configuration.

Also note that I mostly reused the equivalent swh.lister.pattern.Lister.from_config*.
I did not refactor the common behavior (to avoid throwing another dependency in the
mix). That could always be refactored later.

Unfortunately the diff is a "tag" big but we can't really work around it...

(inspired by both the work on listers and the configuration system work)

Impacts:

Related to T1410

Diff Detail

Repository
rDLDBASE Generic VCS/Package Loader
Branch
update-config
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 19263
Build 29874: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 29873: arc lint + arc unit

Event Timeline

ardumont created this revision.Feb 12 2021, 4:11 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptFeb 12 2021, 4:11 PM
swh-public-ci added a comment.Feb 12 2021, 4:12 PM

Build is green

Patch application report for D5071 (id=18096)

Rebasing onto 7ea69e2b70...

Current branch diff-target is up to date.
Changes applied before test
commit ee61ed44e4a5608d11ce7f7cac34a292625875cc
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Feb 8 09:04:17 2021 +0100

    Unify loader instantiation
    
    This unifies and centralizes the instantiation the same way the lister does.
    
    This introduces a new base class `swh.loader.pattern.Loader` for all loaders whose only
    concern for now is to instantiate loaders from either a configuration dict or a
    configuration file.
    
    This simplify instantiation in celery task code and avoids duplicating the configuration
    load in each loader constructor.
    
    The end goal is to simplify the future refactoring on configuration. Now, we will only
    have to adapt the Loader class when we finally start simplifying uniformly the
    configuration.

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

Harbormaster completed remote builds in B19204: Diff 18096.Feb 12 2021, 4:12 PM
ardumont requested review of this revision.Feb 12 2021, 4:12 PM
ardumont edited the summary of this revision. (Show Details)Feb 12 2021, 5:08 PM
ardumont added a project: Core Loader.
ardumont edited the summary of this revision. (Show Details)Feb 12 2021, 5:51 PM
ardumont added a task: T1410: Kill implicit configuration: new configuration scheme.
ardumont edited the summary of this revision. (Show Details)Feb 12 2021, 8:12 PM
ardumont edited the summary of this revision. (Show Details)Feb 12 2021, 8:17 PM
vlorentz accepted this revision.Feb 15 2021, 4:43 PM
This revision is now accepted and ready to land.Feb 15 2021, 4:43 PM
ardumont edited the summary of this revision. (Show Details)Feb 15 2021, 6:12 PM
ardumont updated this revision to Diff 18127.Feb 16 2021, 9:53 AM
  • Add missing test on Deposit.from_configfile
  • Drop unneeded conditional in cli
swh-public-ci added a comment.Feb 16 2021, 9:54 AM

Build has FAILED

Patch application report for D5071 (id=18127)

Rebasing onto 7ea69e2b70...

Current branch diff-target is up to date.
Changes applied before test
commit 4dd956dcae9ba861b098e1ceba2e502233ea4057
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Feb 8 09:04:17 2021 +0100

    Unify loader instantiation
    
    This unifies and centralizes the instantiation the same way the lister does.
    
    This introduces a new base class `swh.loader.pattern.Loader` for all loaders whose only
    concern for now is to instantiate loaders from either a configuration dict or a
    configuration file.
    
    This simplify instantiation in celery task code and avoids duplicating the configuration
    load in each loader constructor.
    
    The end goal is to simplify the future refactoring on configuration. Now, we will only
    have to adapt the Loader class when we finally start simplifying uniformly the
    configuration.

Link to build: https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/372/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/372/console

Harbormaster failed remote builds in B19231: Diff 18127!Feb 16 2021, 9:54 AM
ardumont updated this revision to Diff 18128.Feb 16 2021, 9:58 AM

Fix unused import

swh-public-ci added a comment.Feb 16 2021, 10:00 AM

Build is green

Patch application report for D5071 (id=18128)

Rebasing onto 7ea69e2b70...

Current branch diff-target is up to date.
Changes applied before test
commit c0301f5365e01275d6175cdcc2cd540102d2b73a
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Feb 8 09:04:17 2021 +0100

    Unify loader instantiation
    
    This unifies and centralizes the instantiation the same way the lister does.
    
    This introduces a new base class `swh.loader.pattern.Loader` for all loaders whose only
    concern for now is to instantiate loaders from either a configuration dict or a
    configuration file.
    
    This simplify instantiation in celery task code and avoids duplicating the configuration
    load in each loader constructor.
    
    The end goal is to simplify the future refactoring on configuration. Now, we will only
    have to adapt the Loader class when we finally start simplifying uniformly the
    configuration.

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

Harbormaster completed remote builds in B19232: Diff 18128.Feb 16 2021, 10:00 AM
ardumont updated this revision to Diff 18132.Feb 16 2021, 10:18 AM

Add missing test on cli run edge case

swh-public-ci added a comment.Feb 16 2021, 10:20 AM

Build is green

Patch application report for D5071 (id=18132)

Rebasing onto 7ea69e2b70...

Current branch diff-target is up to date.
Changes applied before test
commit 0781cdfd9ba4b9432869201299abc4572b7b00c5
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Feb 8 09:04:17 2021 +0100

    Unify loader instantiation
    
    This unifies and centralizes the instantiation the same way the lister does.
    
    This introduces a new base class `swh.loader.pattern.Loader` for all loaders whose only
    concern for now is to instantiate loaders from either a configuration dict or a
    configuration file.
    
    This simplify instantiation in celery task code and avoids duplicating the configuration
    load in each loader constructor.
    
    The end goal is to simplify the future refactoring on configuration. Now, we will only
    have to adapt the Loader class when we finally start simplifying uniformly the
    configuration.

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

Harbormaster completed remote builds in B19236: Diff 18132.Feb 16 2021, 10:20 AM
olasd added a subscriber: olasd.Feb 16 2021, 5:32 PM

This is great, thanks!

Are the new pattern module / pattern.Loader class really needed? It looks like these methods could live in the BaseLoader class directly.

ardumont added a comment.Feb 16 2021, 5:35 PM

Are the new pattern module / pattern.Loader class really needed? It looks like these methods could live in the BaseLoader class directly.

PackageLoader does not actually inherit from BaseLoader so that's why i did not go that way.

olasd added a comment.Feb 16 2021, 6:07 PM

@ardumont points out that the base PackageLoader doesn't inherit from BaseLoader, which explains the new (common) base class. I think the new class could just as well be next to BaseLoader, and doesn't warrant the introduction of a pattern module.

I wonder what would break if the new methods were just put in BaseLoader and the PackageLoader was made to inherit BaseLoader (now that the ABC has been converted to NotImplemented methods) instead of deepening the dependency graph for all loaders.

swh/loader/package/loader.py
128–129

You're dropping the only use of these parameters in tests, so you might as well drop them altogether.

ardumont added a comment.Feb 16 2021, 6:20 PM

@ardumont points out that the base PackageLoader doesn't inherit from BaseLoader, which explains the new (common) base class. I think the new class could just as well be next to BaseLoader, and doesn't warrant the introduction of a pattern module.

Oh, yes, totally, that could go in core.loader indeed.

I wonder what would break if the new methods were just put in BaseLoader and the PackageLoader was made to inherit BaseLoader

I don't think anything would break. I'm just not sure the "indirection" would be clear in terms of code readability...

(now that the ABC has been converted to NotImplemented methods) instead of deepening the dependency graph for all loaders.

Funny how I did not see it as deepening the graph (i see the point now though ;)

For me it creates indirection instead. The base behavior (BaseLoader) is not shared by the package loader. (Granted, that's already the case for DVCSLoader inheriting from BaseLoader...).
So i thought it was inappropriate to do so.

Like i said, i'll try.

ardumont updated this revision to Diff 18159.Feb 16 2021, 6:21 PM

Adapt according to review:

  • Drop swh.loader.pattern and move class Loader in swh.loader.core.loader module
  • Drop unneeded self.create_authorities, self.create_fetchers

Thanks.

(I'll try next the inheritance)

swh-public-ci added a comment.Feb 16 2021, 6:23 PM

Build is green

Patch application report for D5071 (id=18159)

Rebasing onto 7ea69e2b70...

Current branch diff-target is up to date.
Changes applied before test
commit 0c36690f10b26a9916150a5606d3de2ad2d97cb6
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Feb 8 09:04:17 2021 +0100

    Unify loader instantiation
    
    This unifies and centralizes the instantiation the same way the lister does.
    
    This introduces a new base class `swh.loader.pattern.Loader` for all loaders whose only
    concern for now is to instantiate loaders from either a configuration dict or a
    configuration file.
    
    This simplify instantiation in celery task code and avoids duplicating the configuration
    load in each loader constructor.
    
    The end goal is to simplify the future refactoring on configuration. Now, we will only
    have to adapt the Loader class when we finally start simplifying uniformly the
    configuration.

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

Harbormaster completed remote builds in B19263: Diff 18159.Feb 16 2021, 6:23 PM
ardumont added a comment.Feb 16 2021, 6:33 PM

I wonder what would break if the new methods were just put in BaseLoader and the PackageLoader was made to inherit BaseLoader

I don't think anything would break. I'm just not sure the "indirection" would be clear in terms of code readability...

I was wrong. mypy is not happy.

Some signatures would need changing, notably the load, prepare, prepare_origin_visit to drop the spurious (i think) {*args, **kwargs} we are declaring.
(I don't think they are still used any more now, aside for the extra logging arguments)

But still, i'd be confortable if we go that way to do it in another diff (maybe).

$ tox -e mypy
GLOB sdist-make: /home/tony/work/inria/repo/swh/swh-environment/swh-loader-core/setup.py
...
mypy run-test: commands[0] | mypy swh
swh/loader/package/loader.py:289: error: Signature of "load" incompatible with supertype "BaseLoader"
swh/loader/package/loader.py:381: error: Argument "date" to "OriginVisit" has incompatible type "Optional[datetime]"; expected "datetime"
swh/loader/package/loader.py:570: error: Argument "discovery_date" to "RawExtrinsicMetadata" has incompatible type "Optional[datetime]"; expected "datetime"
swh/loader/package/loader.py:692: error: Argument "discovery_date" to "RawExtrinsicMetadata" has incompatible type "Optional[datetime]"; expected "datetime"
swh/loader/package/loader.py:727: error: Argument "discovery_date" to "RawExtrinsicMetadata" has incompatible type "Optional[datetime]"; expected "datetime"
swh/loader/package/loader.py:756: error: Argument "discovery_date" to "RawExtrinsicMetadata" has incompatible type "Optional[datetime]"; expected "datetime"
swh/loader/package/pypi/loader.py:151: error: Item "None" of "Optional[datetime]" has no attribute "isoformat"
swh/loader/package/npm/loader.py:177: error: Item "None" of "Optional[datetime]" has no attribute "isoformat"
swh/loader/package/nixguix/loader.py:218: error: Item "None" of "Optional[datetime]" has no attribute "isoformat"
swh/loader/package/deposit/loader.py:208: error: Item "None" of "Optional[datetime]" has no attribute "isoformat"
swh/loader/package/deposit/loader.py:244: error: Signature of "load" incompatible with supertype "BaseLoader"
swh/loader/package/debian/loader.py:237: error: Item "None" of "Optional[datetime]" has no attribute "isoformat"
swh/loader/package/cran/loader.py:129: error: Item "None" of "Optional[datetime]" has no attribute "isoformat"
swh/loader/package/archive/loader.py:170: error: Item "None" of "Optional[datetime]" has no attribute "isoformat"
Found 14 errors in 8 files (checked 71 source files)
ardumont added a child revision: D5088: core.loader: Merge Loader into BaseLoader.Feb 16 2021, 6:49 PM
ardumont added a comment.Feb 16 2021, 6:52 PM
In D5071#128386, @ardumont wrote:

I wonder what would break if the new methods were just put in BaseLoader and the PackageLoader was made to inherit BaseLoader

I don't think anything would break. I'm just not sure the "indirection" would be clear in terms of code readability...

I was wrong. mypy is not happy.

Some signatures would need changing, notably the load, prepare, prepare_origin_visit to drop the spurious (i think) {*args, **kwargs} we are declaring.
(I don't think they are still used any more now, aside for the extra logging arguments)

But still, i'd be confortable if we go that way to do it in another diff (maybe).

$ tox -e mypy
GLOB sdist-make: /home/tony/work/inria/repo/swh/swh-environment/swh-loader-core/setup.py
...
mypy run-test: commands[0] | mypy swh
swh/loader/package/loader.py:289: error: Signature of "load" incompatible with supertype "BaseLoader"
swh/loader/package/loader.py:381: error: Argument "date" to "OriginVisit" has incompatible type "Optional[datetime]"; expected "datetime"
swh/loader/package/loader.py:570: error: Argument "discovery_date" to "RawExtrinsicMetadata" has incompatible type "Optional[datetime]"; expected "datetime"
swh/loader/package/loader.py:692: error: Argument "discovery_date" to "RawExtrinsicMetadata" has incompatible type "Optional[datetime]"; expected "datetime"
swh/loader/package/loader.py:727: error: Argument "discovery_date" to "RawExtrinsicMetadata" has incompatible type "Optional[datetime]"; expected "datetime"
swh/loader/package/loader.py:756: error: Argument "discovery_date" to "RawExtrinsicMetadata" has incompatible type "Optional[datetime]"; expected "datetime"
swh/loader/package/pypi/loader.py:151: error: Item "None" of "Optional[datetime]" has no attribute "isoformat"
swh/loader/package/npm/loader.py:177: error: Item "None" of "Optional[datetime]" has no attribute "isoformat"
swh/loader/package/nixguix/loader.py:218: error: Item "None" of "Optional[datetime]" has no attribute "isoformat"
swh/loader/package/deposit/loader.py:208: error: Item "None" of "Optional[datetime]" has no attribute "isoformat"
swh/loader/package/deposit/loader.py:244: error: Signature of "load" incompatible with supertype "BaseLoader"
swh/loader/package/debian/loader.py:237: error: Item "None" of "Optional[datetime]" has no attribute "isoformat"
swh/loader/package/cran/loader.py:129: error: Item "None" of "Optional[datetime]" has no attribute "isoformat"
swh/loader/package/archive/loader.py:170: error: Item "None" of "Optional[datetime]" has no attribute "isoformat"
Found 14 errors in 8 files (checked 71 source files)

This is actually manageable in D5088 (kept separate from this).
I'm not sure we want to land this though.
you tell me ;)

ardumont edited the summary of this revision. (Show Details)Feb 16 2021, 6:57 PM
olasd accepted this revision.Feb 16 2021, 6:57 PM
In D5071#128386, @ardumont wrote:

I wonder what would break if the new methods were just put in BaseLoader and the PackageLoader was made to inherit BaseLoader

I don't think anything would break. I'm just not sure the "indirection" would be clear in terms of code readability...

I was wrong. mypy is not happy.

Some signatures would need changing, notably the load, prepare, prepare_origin_visit to drop the spurious (i think) {*args, **kwargs} we are declaring.
(I don't think they are still used any more now, aside for the extra logging arguments)

But still, i'd be confortable if we go that way to do it in another diff (maybe).

Yeah, that makes sense.

I'm fine with this standalone change, we can do the base class refactoring in another diff.

ardumont updated this revision to Diff 18167.Feb 16 2021, 7:09 PM

Rework commit message (aligns with diff)

swh-public-ci added a comment.Feb 16 2021, 7:11 PM

Build is green

Patch application report for D5071 (id=18167)

Rebasing onto 7ea69e2b70...

Current branch diff-target is up to date.
Changes applied before test
commit 7116bb75897ae878f68679e86aaa5aec6cc507be
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Feb 8 09:04:17 2021 +0100

    Unify loader instantiation
    
    This unifies and centralizes the instantiation the same way the lister does.
    
    This introduces a new base class swh.loader.core.loader.Loader for all loaders whose
    only concern for now is to instantiate loaders from either a configuration dict or a
    configuration file.
    
    This simplifies instantiation in celery task code and avoids duplicating the
    configuration load in each loader constructor.
    
    The end goal is to simplify the future refactoring on configuration. With the following,
    we will only have to adapt the Loader class when we start simplifying uniformly the
    configuration.
    
    Also note that I mostly reused the equivalent `swh.lister.pattern.Lister.from_config*`.
    I did not refactor the common behavior (to avoid throwing another dependency in the
    mix). That could always be refactored later.
    
    (inspired by both the work on listers and the configuration system work)
    
    Related to T1410

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

Harbormaster completed remote builds in B19271: Diff 18167.Feb 16 2021, 7:11 PM
Closed by commit rDLDBASE7116bb75897a: Unify loader instantiation (authored by ardumont). · Explain WhyFeb 17 2021, 12:03 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDLDBASE7116bb75897a: Unify loader instantiation.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
loader/
33 lines96%
Loading...
core/
115 lines83%
Loading...
tests/
47 lines96%
Loading...
package/
archive/
7 lines100%
Loading...
5 lines100%
Loading...
tests/
74 lines100%
Loading...
12 lines100%
Loading...
cran/
13 lines98%
Loading...
4 lines100%
Loading...
tests/
42 lines100%
Loading...
17 lines100%
Loading...
debian/
14 lines94%
Loading...
3 lines100%
Loading...
tests/
65 lines100%
Loading...
12 lines100%
Loading...
deposit/
35 lines94%
Loading...
4 lines100%
Loading...
tests/
9 lines100%
Loading...
45 lines100%
Loading...
15 lines100%
Loading...
38 lines97%
Loading...
nixguix/
16 lines99%
Loading...
4 lines100%
Loading...
tests/
114 lines100%
Loading...
20 lines100%
Loading...
npm/
10 lines93%
Loading...
4 lines100%
Loading...
tests/
89 lines98%
Loading...
10 lines100%
Loading...
pypi/
10 lines98%
Loading...
4 lines100%
Loading...
tests/
160 lines100%
Loading...
10 lines100%
Loading...
tests/
8 lines97%
Loading...
47 lines100%
Loading...
28 lines100%
Loading...
tests/
4 lines100%
Loading...
65 lines100%
Loading...
11 lines100%
Loading...

Diff 18159

View Options

swh/loader/cli.py

Loading...
View Options

swh/loader/core/loader.py

Loading...
View Options

swh/loader/core/tests/test_loader.py

Loading...
View Options

swh/loader/package/archive/loader.py

Loading...
View Options

swh/loader/package/archive/tasks.py

Loading...
View Options

swh/loader/package/archive/tests/test_archive.py

Loading...
View Options

swh/loader/package/archive/tests/test_tasks.py

Loading...
View Options

swh/loader/package/cran/loader.py

Loading...
View Options

swh/loader/package/cran/tasks.py

Loading...
View Options

swh/loader/package/cran/tests/test_cran.py

Loading...
View Options

swh/loader/package/cran/tests/test_tasks.py

Loading...
View Options

swh/loader/package/debian/loader.py

Loading...
View Options

swh/loader/package/debian/tasks.py

Loading...
View Options

swh/loader/package/debian/tests/test_debian.py

Loading...
View Options

swh/loader/package/debian/tests/test_tasks.py

Loading...
View Options

swh/loader/package/deposit/loader.py

Loading...
View Options

swh/loader/package/deposit/tasks.py

Loading...
View Options

swh/loader/package/deposit/tests/conftest.py

Loading...
View Options

swh/loader/package/deposit/tests/test_deposit.py

Loading...
View Options

swh/loader/package/deposit/tests/test_tasks.py

Loading...
View Options

swh/loader/package/loader.py

Loading...
View Options

swh/loader/package/nixguix/loader.py

Loading...
View Options

swh/loader/package/nixguix/tasks.py

Loading...
View Options

swh/loader/package/nixguix/tests/test_nixguix.py

Loading...
View Options

swh/loader/package/nixguix/tests/test_tasks.py

Loading...
View Options

swh/loader/package/npm/loader.py

Loading...
View Options

swh/loader/package/npm/tasks.py

Loading...
View Options

swh/loader/package/npm/tests/test_npm.py

Loading...
View Options

swh/loader/package/npm/tests/test_tasks.py

Loading...
View Options

swh/loader/package/pypi/loader.py

Loading...
View Options

swh/loader/package/pypi/tasks.py

Loading...
View Options

swh/loader/package/pypi/tests/test_pypi.py

Loading...
View Options

swh/loader/package/pypi/tests/test_tasks.py

Loading...
View Options

swh/loader/package/tests/test_loader.py

Loading...
View Options

swh/loader/package/tests/test_loader_metadata.py

Loading...
View Options

swh/loader/pytest_plugin.py

Loading...
View Options

swh/loader/tests/conftest.py

Loading...
View Options

swh/loader/tests/test_cli.py

Loading...
View Options

swh/loader/tests/test_init.py

Loading...
About · Developer information · Legal