Page MenuHomeSoftware Heritage
Differential D7694

tasks: Simplify implementation and make visit_date parameter optional
ClosedPublic
Actions

Authored by anlambert on Apr 27 2022, 11:06 AM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T4187: Pass forge type to loaders
Commits
rDLDG05242cd461fd: tasks: Simplify implementation and make visit_date parameter optional
Summary

Recent changes in swh-scheduler add new parameters to the celery tasks
produced from swh.scheduler.model.ListedOrigin instances.

So ensure to handle any new parameters by not hardcoding the expected
ones in task signatures.

Rename date parameter to visit_date in from disk loader tasks and
make it non mandatory.

Add new tests checking task parameters produced from ListedOrigin
instances do no raise error when attempting to create a git loader.

Related to T4187

Diff Detail

Repository
rDLDG Git loader
Branch
simplify-tasks
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 28873
Build 45128: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 45127: arc lint + arc unit

Event Timeline

anlambert created this revision.Apr 27 2022, 11:06 AM
Herald added a reviewer: Reviewers. · View Herald TranscriptApr 27 2022, 11:06 AM
swh-public-ci added a comment.Apr 27 2022, 11:09 AM

Build is green

Patch application report for D7694 (id=27811)

Rebasing onto b34d31e600...

Current branch diff-target is up to date.
Changes applied before test
commit 005b5f476a43a8616fc3ea850d6535f8f8e79639
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Wed Apr 27 11:01:12 2022 +0200

    tasks: Simplify implementation and add tests for listed origins
    
    Recent changes in swh-scheduler add new parameters to the celery tasks
    produced from swh.scheduler.model.ListedOrigin instances.
    
    So ensure to handle any new parameters by not hardcoding the expected
    ones in task signatures.
    
    Also remove explicit setting of visit_date if none has been provided
    as task parameter as it is already handled in loader constructor.
    
    Add new tests checking task parameters produced from ListedOrigin
    instances do no raise error when attempting to create a git loader.
    
    Related to T4187

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

Harbormaster completed remote builds in B28872: Diff 27811.Apr 27 2022, 11:09 AM
anlambert requested review of this revision.Apr 27 2022, 11:09 AM
vlorentz added a subscriber: vlorentz.Apr 27 2022, 11:14 AM

Remove *args, it's a footgun to allow unnamed arguments over the scheduler/Celery.

Also remove explicit setting of visit_date if none has been provided
as task parameter as it is already handled in loader constructor.

Where? All I see is that it expects a datetime: https://forge.softwareheritage.org/source/swh-loader-git/browse/master/swh/loader/git/from_disk.py$99

anlambert added a comment.Apr 27 2022, 11:26 AM

Remove *args, it's a footgun to allow unnamed arguments over the scheduler/Celery.

Ack, will do the change on the svn loader too then.

Where? All I see is that it expects a datetime: https://forge.softwareheritage.org/source/swh-loader-git/browse/master/swh/loader/git/from_disk.py$99

What I mean is that we never use the visit_date argument for celery tasks, the visit date value is set as current one in swh.loader.core.loader.BaseLoader constructor.
So there is no need to parse a string date in tasks implementation.

anlambert updated this revision to Diff 27812.Apr 27 2022, 11:31 AM

Remove unnamed arguments use in celery tasks

swh-public-ci added a comment.Apr 27 2022, 11:34 AM

Build is green

Patch application report for D7694 (id=27812)

Rebasing onto b34d31e600...

Current branch diff-target is up to date.
Changes applied before test
commit dbd991ecb361a7482da65ff6a0b4cbeba91978d5
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Wed Apr 27 11:01:12 2022 +0200

    tasks: Simplify implementation and add tests for listed origins
    
    Recent changes in swh-scheduler add new parameters to the celery tasks
    produced from swh.scheduler.model.ListedOrigin instances.
    
    So ensure to handle any new parameters by not hardcoding the expected
    ones in task signatures.
    
    Also remove explicit setting of visit_date if none has been provided
    as task parameter as it is already handled in loader constructor.
    
    Add new tests checking task parameters produced from ListedOrigin
    instances do no raise error when attempting to create a git loader.
    
    Related to T4187

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

Harbormaster completed remote builds in B28873: Diff 27812.Apr 27 2022, 11:34 AM
vlorentz added a comment.Apr 27 2022, 11:35 AM

The date is indeed optional, but it must be parsed if provided.

vlorentz added a comment.Apr 27 2022, 11:45 AM

Actually, I think it would make sense to keep it mandatory from Celery tasks; as visit dates when loading from disk should be the date the local git repo was downloaded, not the date it was loaded.

anlambert updated this revision to Diff 27813.Apr 27 2022, 11:51 AM

Restore parsing of visit_date parameter for from disk loader tasks

anlambert added a comment.Apr 27 2022, 11:54 AM

Actually, I think it would make sense to keep it mandatory from Celery tasks; as visit dates when loading from disk should be the date the local git repo was downloaded, not the date it was loaded.

I did not make the visit_date parameter mandatory but restore its parsing in the tasks for from disk loaders.

swh-public-ci added a comment.Apr 27 2022, 11:54 AM

Build is green

Patch application report for D7694 (id=27813)

Rebasing onto b34d31e600...

Current branch diff-target is up to date.
Changes applied before test
commit 8d3e3dd7f23f83444f3a4266656a2be41af55fcb
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Wed Apr 27 11:01:12 2022 +0200

    tasks: Simplify implementation and add tests for listed origins
    
    Recent changes in swh-scheduler add new parameters to the celery tasks
    produced from swh.scheduler.model.ListedOrigin instances.
    
    So ensure to handle any new parameters by not hardcoding the expected
    ones in task signatures.
    
    Also remove explicit setting of visit_date if none has been provided
    as task parameter as it is already handled in loader constructor.
    
    Add new tests checking task parameters produced from ListedOrigin
    instances do no raise error when attempting to create a git loader.
    
    Related to T4187

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

Harbormaster completed remote builds in B28874: Diff 27813.Apr 27 2022, 11:54 AM
vlorentz added a comment.Apr 27 2022, 11:55 AM

Thanks. Could you mention the behavior change in the commit/diff title?

anlambert updated this revision to Diff 27814.Apr 27 2022, 12:08 PM

Update commit message

anlambert edited the summary of this revision. (Show Details)Apr 27 2022, 12:08 PM
In D7694#200395, @vlorentz wrote:

Thanks. Could you mention the behavior change in the commit/diff title?

I updated the commit message about the date parameter changes.

swh-public-ci added a comment.Apr 27 2022, 12:11 PM

Build is green

Patch application report for D7694 (id=27814)

Rebasing onto b34d31e600...

Current branch diff-target is up to date.
Changes applied before test
commit 4b8144a03071180a17d8ac24ad8d6187546eb10c
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Wed Apr 27 11:01:12 2022 +0200

    tasks: Simplify implementation and add tests for listed origins
    
    Recent changes in swh-scheduler add new parameters to the celery tasks
    produced from swh.scheduler.model.ListedOrigin instances.
    
    So ensure to handle any new parameters by not hardcoding the expected
    ones in task signatures.
    
    Rename date parameter to visit_date in from disk loader tasks and
    make it non mandatory.
    
    Add new tests checking task parameters produced from ListedOrigin
    instances do no raise error when attempting to create a git loader.
    
    Related to T4187

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

Harbormaster completed remote builds in B28875: Diff 27814.Apr 27 2022, 12:11 PM
vlorentz added a comment.Apr 27 2022, 12:28 PM

Sorry to be annoying about this, but it should be in the title. eg. tasks: Simplify implementation and make visit_date optional; the change in behavior is more important than tests when reading history

anlambert updated this revision to Diff 27816.Apr 27 2022, 1:16 PM

Update commit message title

anlambert retitled this revision from tasks: Simplify implementation and add tests for listed origins to tasks: Simplify implementation and make visit_date parameter optional.Apr 27 2022, 1:17 PM
swh-public-ci added a comment.Apr 27 2022, 1:19 PM

Build is green

Patch application report for D7694 (id=27816)

Rebasing onto b34d31e600...

Current branch diff-target is up to date.
Changes applied before test
commit 05242cd461fdefd9013228b61a9318fe5c1520f6
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Wed Apr 27 11:01:12 2022 +0200

    tasks: Simplify implementation and make visit_date parameter optional
    
    Recent changes in swh-scheduler add new parameters to the celery tasks
    produced from swh.scheduler.model.ListedOrigin instances.
    
    So ensure to handle any new parameters by not hardcoding the expected
    ones in task signatures.
    
    Rename date parameter to visit_date in from disk loader tasks and
    make it non mandatory.
    
    Add new tests checking task parameters produced from ListedOrigin
    instances do no raise error when attempting to create a git loader.
    
    Related to T4187

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

Harbormaster completed remote builds in B28877: Diff 27816.Apr 27 2022, 1:19 PM
vlorentz accepted this revision.Apr 27 2022, 1:22 PM
This revision is now accepted and ready to land.Apr 27 2022, 1:22 PM
Closed by commit rDLDG05242cd461fd: tasks: Simplify implementation and make visit_date parameter optional (authored by anlambert). · Explain WhyApr 27 2022, 1:41 PM
This revision was automatically updated to reflect the committed changes.
anlambert added a commit: rDLDG05242cd461fd: tasks: Simplify implementation and make visit_date parameter optional.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
1 line--
swh/
loader/
git/
30 lines100%
Loading...
tests/
113 lines100%
Loading...

Diff 27812

View Options

requirements-test.txt

Loading...
View Options

swh/loader/git/tasks.py

Loading...
View Options

swh/loader/git/tests/test_tasks.py

Loading...
About · Developer information · Legal