Page MenuHomeSoftware Heritage
Differential D3921

cli: speedup the `swh` cli command startup time
ClosedPublic
Actions

Authored by douardda on Sep 10 2020, 4:25 PM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T2575: Investigate if/how we could improve `swh` cli command startup time
Commits
rDSTOfd6d72f98ee8: cli: speedup the `swh` cli command startup time
Summary

by moving import statements in functions, as well as preventing
the loading of swh.storage.interface in swh.storage unless needed
for type checking.

Related to T2575.

Diff Detail

Unit TestsFailed
View All

TimeTest
1 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.storage.tests.test_cli::test_replay
@pytest.fixture def swh_storage(): """An swh-storage object that gets injected into the CLI functions."""
3 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.storage.fixer::swh.storage.fixer._fix_content
1 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.storage.fixer::swh.storage.fixer._fix_origin
3 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.storage.fixer::swh.storage.fixer._fix_origin_visit
3 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.storage.fixer::swh.storage.fixer._fix_revision
View Full Test Results (1 Failed · 747 Passed · 23 Skipped)

Event Timeline

douardda created this revision.Sep 10 2020, 4:25 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptSep 10 2020, 4:25 PM
swh-public-ci added a comment.Sep 10 2020, 4:30 PM

Build has FAILED

Patch application report for D3921 (id=13781)

Rebasing onto d24a1e7732...

First, rewinding head to replay your work on top of it...
Applying: cli: speedup the `swh` cli command startup time
Changes applied before test
commit 11fb17cb7ec5f5d801fd73cb68376d72b1fbb631
Author: David Douard <david.douard@sdfa3.org>
Date:   Thu Sep 10 15:15:33 2020 +0200

    cli: speedup the `swh` cli command startup time
    
    by moving import statements in functions, as well as preventing
    the loading of swh.storage.interface in swh.storage unless needed
    for type checking.
    
    Related to T2575.

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

Harbormaster failed remote builds in B15008: Diff 13781!Sep 10 2020, 4:30 PM
vlorentz added a subscriber: vlorentz.Sep 10 2020, 4:45 PM
vlorentz added inline comments.
swh/storage/__init__.py
26

string

73

string

douardda updated this revision to Diff 13811.Sep 10 2020, 5:38 PM

use string annotations

swh-public-ci added a comment.Sep 10 2020, 5:42 PM

Build has FAILED

Patch application report for D3921 (id=13811)

Rebasing onto d24a1e7732...

First, rewinding head to replay your work on top of it...
Applying: cli: speedup the `swh` cli command startup time
Changes applied before test
commit 2822031b8db2d9bd2510b5df7b63af0bde551122
Author: David Douard <david.douard@sdfa3.org>
Date:   Thu Sep 10 15:15:33 2020 +0200

    cli: speedup the `swh` cli command startup time
    
    by moving import statements in functions, as well as preventing
    the loading of swh.storage.interface in swh.storage unless needed
    for type checking.
    
    Related to T2575.

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

Harbormaster failed remote builds in B15036: Diff 13811!Sep 10 2020, 5:42 PM
douardda updated this revision to Diff 13814.Sep 10 2020, 6:41 PM

pfff

swh-public-ci added a comment.Sep 10 2020, 6:46 PM

Build has FAILED

Patch application report for D3921 (id=13814)

Rebasing onto d24a1e7732...

First, rewinding head to replay your work on top of it...
Applying: cli: speedup the `swh` cli command startup time
Changes applied before test
commit ab81a73d2ef97f38a730aa0a99b1de860d7c42d2
Author: David Douard <david.douard@sdfa3.org>
Date:   Thu Sep 10 15:15:33 2020 +0200

    cli: speedup the `swh` cli command startup time
    
    by moving import statements in functions, as well as preventing
    the loading of swh.storage.interface in swh.storage unless needed
    for type checking.
    
    Related to T2575.

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

vlorentz accepted this revision.Sep 10 2020, 6:46 PM
This revision is now accepted and ready to land.Sep 10 2020, 6:46 PM
Harbormaster failed remote builds in B15039: Diff 13814!Sep 10 2020, 6:46 PM
douardda updated this revision to Diff 13829.Sep 11 2020, 11:16 AM

rebase + fix test_cli

swh-public-ci added a comment.Sep 11 2020, 11:21 AM

Build is green

Patch application report for D3921 (id=13829)

Rebasing onto d24a1e7732...

Current branch diff-target is up to date.
Changes applied before test
commit fd6d72f98ee8c79feaced17e44fe1076f71d0595
Author: David Douard <david.douard@sdfa3.org>
Date:   Thu Sep 10 15:15:33 2020 +0200

    cli: speedup the `swh` cli command startup time
    
    by moving import statements in functions, as well as preventing
    the loading of swh.storage.interface in swh.storage unless needed
    for type checking.
    
    Related to T2575.

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

Harbormaster completed remote builds in B15054: Diff 13829.Sep 11 2020, 11:21 AM
Closed by commit rDSTOfd6d72f98ee8: cli: speedup the `swh` cli command startup time (authored by douardda). · Explain WhySep 11 2020, 11:45 AM
This revision was automatically updated to reflect the committed changes.
douardda added a commit: rDSTOfd6d72f98ee8: cli: speedup the `swh` cli command startup time.

Revision Contents
Changeset List

PathSize
swh/
storage/
9 lines
17 lines

Diff 13811

View Options

swh/storage/__init__.py

Show First 20 Lines • Show All 59 Lines • ▼ Show 20 Lines def get_package_info(self, url):
# currently only use the first one, but if the first one # currently only use the first one, but if the first one
# fails, we should try the second one and so on. # fails, we should try the second one and so on.
integrity = self._integrityByUrl[url] integrity = self._integrityByUrl[url]
yield url, {"url": url, "raw": {"url": url, "integrity": integrity}} yield url, {"url": url, "raw": {"url": url, "integrity": integrity}}
def resolve_revision_from( def resolve_revision_from(
self, known_artifacts: Dict, artifact_metadata: Dict self, known_artifacts: Dict, artifact_metadata: Dict
) -> Optional[bytes]: ) -> Optional[bytes]:
for rev_id, known_artifact in known_artifacts.items(): for rev_id, known_artifact in known_artifacts.items():
try:
lewoUnsubmitted
Not Done
Inline Actions

We should catch a keyError exception instead since it could come from several keys.

lewo: We should catch a `keyError` exception instead since it could come from several keys.
ardumontAuthorUnsubmitted
Done
Inline Actions

yes

ardumont: yes
douarddaUnsubmitted
Not Done
Inline Actions

I agree a try/except KeyError is better here.

Also why exit the for loop? Isn't there any chance the searched item is found later in the for loop?

douardda: I agree a try/except KeyError is better here. Also why exit the for loop? Isn't there any…
ardumontAuthorUnsubmitted
Done
Inline Actions

Yes, i changed it locally already. Thanks.

I can only assume i was thinking (partially wrongly), the other revisions must
be in the same bad format. It was true in staging when i first hit that issue,
that's possibly not the case in production.

ardumont: Yes, i changed it locally already. Thanks. I can only assume i was thinking (partially…
known_integrity = known_artifact["extrinsic"]["raw"]["integrity"] known_integrity = known_artifact["extrinsic"]["raw"]["integrity"]
lewoUnsubmitted
Done
Inline Actions

When the sources.json file is parsed, we check the integrity attribute is present. So, I think it should never be missing.
In this case, I think it would improve code clearness to move this expression out of the try block.

lewo: When the sources.json file is parsed, we check the integrity attribute is present. So, I think…
ardumontAuthorUnsubmitted
Done
Inline Actions

ack, adapted accordingly, thx.

ardumont: ack, adapted accordingly, thx.
if artifact_metadata["integrity"] == known_integrity: if artifact_metadata["integrity"] == known_integrity:
return rev_id return rev_id
vlorentzUnsubmitted
Not Done
Inline Actions

I'd rather call it "unexpected" than "divergent"

(divergent evokes an infinite data structure for me)

vlorentz: I'd rather call it "unexpected" than "divergent" (divergent evokes an infinite data structure…
except KeyError:
# metadata field for the revision is not as expected by the loader
# nixguix. We consider this not the right revision
# continue checking the other revisions
continue
return None return None
def extra_branches(self) -> Dict[bytes, Mapping[str, Any]]: def extra_branches(self) -> Dict[bytes, Mapping[str, Any]]:
vlorentzUnsubmitted
Not Done
Inline Actions

why not log.exception? it looks important enough to deserve it

vlorentz: why not `log.exception`? it looks important enough to deserve it
"""We add a branch to the snapshot called 'evaluation' pointing to the """We add a branch to the snapshot called 'evaluation' pointing to the
revision used to generate the sources.json file. This revision revision used to generate the sources.json file. This revision
is specified in the sources.json file itself. For the nixpkgs is specified in the sources.json file itself. For the nixpkgs
origin, this revision is coming from the origin, this revision is coming from the
github.com/nixos/nixpkgs repository. github.com/nixos/nixpkgs repository.
Note this repository is not loaded explicitly. So, this Note this repository is not loaded explicitly. So, this
pointer can target a nonexistent revision for a time. However, pointer can target a nonexistent revision for a time. However,
▲ Show 20 Lines • Show All 101 Lines • Show Last 20 Lines
View Options

swh/storage/cli.py

Show First 20 Lines • Show All 59 Lines • ▼ Show 20 Lines def get_package_info(self, url):
# currently only use the first one, but if the first one # currently only use the first one, but if the first one
# fails, we should try the second one and so on. # fails, we should try the second one and so on.
integrity = self._integrityByUrl[url] integrity = self._integrityByUrl[url]
yield url, {"url": url, "raw": {"url": url, "integrity": integrity}} yield url, {"url": url, "raw": {"url": url, "integrity": integrity}}
def resolve_revision_from( def resolve_revision_from(
self, known_artifacts: Dict, artifact_metadata: Dict self, known_artifacts: Dict, artifact_metadata: Dict
) -> Optional[bytes]: ) -> Optional[bytes]:
for rev_id, known_artifact in known_artifacts.items(): for rev_id, known_artifact in known_artifacts.items():
try:
lewoUnsubmitted
Not Done
Inline Actions

We should catch a keyError exception instead since it could come from several keys.

lewo: We should catch a `keyError` exception instead since it could come from several keys.
ardumontAuthorUnsubmitted
Done
Inline Actions

yes

ardumont: yes
douarddaUnsubmitted
Not Done
Inline Actions

I agree a try/except KeyError is better here.

Also why exit the for loop? Isn't there any chance the searched item is found later in the for loop?

douardda: I agree a try/except KeyError is better here. Also why exit the for loop? Isn't there any…
ardumontAuthorUnsubmitted
Done
Inline Actions

Yes, i changed it locally already. Thanks.

I can only assume i was thinking (partially wrongly), the other revisions must
be in the same bad format. It was true in staging when i first hit that issue,
that's possibly not the case in production.

ardumont: Yes, i changed it locally already. Thanks. I can only assume i was thinking (partially…
known_integrity = known_artifact["extrinsic"]["raw"]["integrity"] known_integrity = known_artifact["extrinsic"]["raw"]["integrity"]
lewoUnsubmitted
Done
Inline Actions

When the sources.json file is parsed, we check the integrity attribute is present. So, I think it should never be missing.
In this case, I think it would improve code clearness to move this expression out of the try block.

lewo: When the sources.json file is parsed, we check the integrity attribute is present. So, I think…
ardumontAuthorUnsubmitted
Done
Inline Actions

ack, adapted accordingly, thx.

ardumont: ack, adapted accordingly, thx.
if artifact_metadata["integrity"] == known_integrity: if artifact_metadata["integrity"] == known_integrity:
return rev_id return rev_id
vlorentzUnsubmitted
Not Done
Inline Actions

I'd rather call it "unexpected" than "divergent"

(divergent evokes an infinite data structure for me)

vlorentz: I'd rather call it "unexpected" than "divergent" (divergent evokes an infinite data structure…
except KeyError:
# metadata field for the revision is not as expected by the loader
# nixguix. We consider this not the right revision
# continue checking the other revisions
continue
return None return None
def extra_branches(self) -> Dict[bytes, Mapping[str, Any]]: def extra_branches(self) -> Dict[bytes, Mapping[str, Any]]:
vlorentzUnsubmitted
Not Done
Inline Actions

why not log.exception? it looks important enough to deserve it

vlorentz: why not `log.exception`? it looks important enough to deserve it
"""We add a branch to the snapshot called 'evaluation' pointing to the """We add a branch to the snapshot called 'evaluation' pointing to the
revision used to generate the sources.json file. This revision revision used to generate the sources.json file. This revision
is specified in the sources.json file itself. For the nixpkgs is specified in the sources.json file itself. For the nixpkgs
origin, this revision is coming from the origin, this revision is coming from the
github.com/nixos/nixpkgs repository. github.com/nixos/nixpkgs repository.
Note this repository is not loaded explicitly. So, this Note this repository is not loaded explicitly. So, this
pointer can target a nonexistent revision for a time. However, pointer can target a nonexistent revision for a time. However,
▲ Show 20 Lines • Show All 101 Lines • Show Last 20 Lines
About · Developer information · Legal