Page MenuHomeSoftware Heritage
Differential D6240

Use extids to filter out already seen revisions across hg origins
ClosedPublic
Actions

Authored by ardumont on Sep 10 2021, 5:27 PM.

Details

Reviewers
olasd
Group Reviewers
Reviewers
Commits
rDLDHG765cb5aefa8b: Add support to filter out already seen revisions across hg origins
Summary

For a given origin, if a previous snapshot exists, this keeps the behavior of filtering
out already seen revisions from that snapshot. Then it goes on and tries to filter the
remaining revisions through the extid mappings for other mercurial origins.

Note: This makes me think back that we need to update the storage interface so the
filtering on the extid_version can happen server side. So as to avoid filtering in the
client code. This is somewhat an independent work from this which got started in a
dedicated diff [1].

[1] D6249

Test Plan

tox

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ardumont edited the summary of this revision. (Show Details)Sep 14 2021, 2:16 PM

Explicit that the current behavior does not compute another snapshot when nothing
changes (thus everything gets filtered out).

To show the new behavior is consistent with it.

Marked as FIXME as i'm not sure we want such behavior though.

swh-public-ci added a comment.Sep 14 2021, 2:18 PM

Build is green

Patch application report for D6240 (id=22644)

Rebasing onto 9be124af21...

Current branch diff-target is up to date.
Changes applied before test
commit 4e018d168fd6e11bc9fa60dfde47f3b7edac9660
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 10 17:00:15 2021 +0200

    Use extids to filter out already seen revisions across hg origins
    
    Note that no longer uses the snapshot to actually reuse the extid filtering.

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

Harbormaster completed remote builds in B23580: Diff 22644.Sep 14 2021, 2:18 PM
ardumont updated this revision to Diff 22645.Sep 14 2021, 2:23 PM

Add back dropped test (stash mix up)

swh-public-ci added a comment.Sep 14 2021, 2:26 PM

Build is green

Patch application report for D6240 (id=22645)

Rebasing onto 9be124af21...

Current branch diff-target is up to date.
Changes applied before test
commit e05d3184d0502a3e3bcfe9789e43834944b2cac9
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 10 17:00:15 2021 +0200

    Use extids to filter out already seen revisions across hg origins
    
    Note that no longer uses the snapshot to actually reuse the extid filtering.

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

Harbormaster completed remote builds in B23581: Diff 22645.Sep 14 2021, 2:26 PM
ardumont edited the summary of this revision. (Show Details)Sep 14 2021, 2:51 PM
ardumont added inline comments.Sep 14 2021, 3:26 PM
swh/loader/mercurial/from_disk.py
377–378

@olasd Heads up, i've kept the old behavior as i currently don't see how to do what you
suggested ("filtering after the history filter"). I gather it's because I don't really
see how to merge consistently the filtered (HgFilteredSet/hgutil.smartset) results .

Maybe the current implementation is enough already. It uses best of both world. If there
is snapshot, then use it to filter out already seen revisions (that will work on smaller
subset, still when using the exitd mapping table, that will nonetheless uses other
fork's potential existing mappings).

Otherwise, let's fallback to filter over all extids for the default case.

What do you think of ^?

I did not yet work on grouping the extid reading calls though (@vlorentz's suggestion),
is that necessary?

ardumont updated this revision to Diff 22652.Sep 14 2021, 3:32 PM
ardumont edited the summary of this revision. (Show Details)

Possibly forgotten comment changes

swh-public-ci added a comment.Sep 14 2021, 3:36 PM

Build is green

Patch application report for D6240 (id=22652)

Rebasing onto 9be124af21...

Current branch diff-target is up to date.
Changes applied before test
commit 80991602c2d0034a976ff389965be41c1ae310df
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 10 17:00:15 2021 +0200

    Use extids to filter out already seen revisions across hg origins
    
    Note that no longer uses the snapshot to actually reuse the extid filtering.

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

Harbormaster completed remote builds in B23588: Diff 22652.Sep 14 2021, 3:36 PM
ardumont updated this revision to Diff 22656.Sep 14 2021, 4:18 PM

Actually filter through snapshot then on the remaining data set, filter out through
extids mapping.

Tests seem still happy

ardumont added inline comments.Sep 14 2021, 4:19 PM
swh/loader/mercurial/from_disk.py
377–378

I've tried to make it 1. then 2. instead of 1. or 2.

  1. being using the snapshot.
  2. being using the global extid mappings
swh-public-ci added a comment.Sep 14 2021, 4:20 PM

Build is green

Patch application report for D6240 (id=22656)

Rebasing onto 9be124af21...

Current branch diff-target is up to date.
Changes applied before test
commit 3976e6fb7031010d2d25b845c0943207aac9bd8e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 10 17:00:15 2021 +0200

    Use extids to filter out already seen revisions across hg origins
    
    Note that no longer uses the snapshot to actually reuse the extid filtering.

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

Harbormaster completed remote builds in B23592: Diff 22656.Sep 14 2021, 4:20 PM
ardumont updated this revision to Diff 22658.Sep 14 2021, 4:54 PM

Adapt docstring and rework commit message.

swh-public-ci added a comment.Sep 14 2021, 4:55 PM

Build is green

Patch application report for D6240 (id=22658)

Rebasing onto 9be124af21...

Current branch diff-target is up to date.
Changes applied before test
commit 7a31286e4971d51bb965826963cb3ce6ffc13aa2
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 10 17:00:15 2021 +0200

    Add support to filter out already seen revisions across hg origins
    
    This uses the extid mapping from the storage to actually achieve this.
    
    Related to T3563

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

Harbormaster completed remote builds in B23594: Diff 22658.Sep 14 2021, 4:55 PM
ardumont edited the summary of this revision. (Show Details)Sep 14 2021, 5:09 PM
ardumont updated this revision to Diff 22659.Sep 14 2021, 5:17 PM

Drop no longer needed conditional

swh-public-ci added a comment.Sep 14 2021, 5:19 PM

Build is green

Patch application report for D6240 (id=22659)

Rebasing onto 9be124af21...

Current branch diff-target is up to date.
Changes applied before test
commit 63904aa241861db0dd0f0aaaaa3454772fd7cbcd
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 10 17:00:15 2021 +0200

    Add support to filter out already seen revisions across hg origins
    
    This uses the extid mapping from the storage to actually achieve this.
    
    Related to T3563

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

Harbormaster completed remote builds in B23595: Diff 22659.Sep 14 2021, 5:19 PM
ardumont added inline comments.Sep 14 2021, 5:22 PM
swh/loader/mercurial/tests/test_from_disk.py
741

@olasd @vlorentz Note this though ^ and the comment below.

olasd added inline comments.Sep 14 2021, 5:25 PM
swh/loader/mercurial/tests/test_from_disk.py
741

A full visit shouldn't be able to have snapshot = None, so, yeah, there's something fishy going on.

ardumont added inline comments.Sep 14 2021, 6:31 PM
swh/loader/mercurial/tests/test_from_disk.py
741

It's the current behavior of the implementation though.

That's one of the reason i added some checks in a previous test
about uneventful visit when you ingest twice a repository without change (line 189).

I'll double check that adaptations in the current master (line 189 onwards in the master tomorrow).
If we need to change this, i gather some breaking tests change though (in another diff though).

ardumont marked an inline comment as done.Sep 15 2021, 10:10 AM
ardumont added inline comments.
swh/loader/mercurial/tests/test_from_disk.py
741

What i meant about current behavior is that when a revision is currently detected as seen.
It's filtered out and does not make it into a snapshot.
So in effect, a new visit of the same origin near enough (that is the origin did not change in between visits), will end up into 1 ingestion with 1 visit full with a snapshot the first time, then for the other visit, this will end up with no snapshot.

While it can be "somewhat" ok for the same origin, it becomes a "nono" for a fork since, for a given order, we won't be able to make a snapshot for forks.

The thing is, if we adapt the behavior to actually have a snapshot with already seen revisions (which would be closer to the truth), that will change the behavior for both scenario (same origin and forks). To do this is something that would be clearer to me and more to the point.
But that's more work [1] [2] (in another diff to make things clearer ;)

What do you think?

[1] in another diff to make things clearer

[2] That begs the question of "do we keep the old implementations? (because *sigh* at the idea of adapting this code as well...)

ardumont mentioned this in D6262: test: Explicit that 2 visits without change ends up with no snapshot.Sep 15 2021, 10:27 AM
ardumont marked 4 inline comments as done.Sep 15 2021, 10:58 AM
ardumont added inline comments.
swh/loader/mercurial/tests/test_from_disk.py
741

See D6262 to explicit the wrong behavior.
And T3571 to actually track and fix it.

I'm unsure about in which order to do this though.
I think landing this and then adapt correctly the snapshot handling after this makes sense.

olasd added inline comments.Sep 15 2021, 11:05 AM
swh/loader/mercurial/tests/test_from_disk.py
741

I think landing this and then adapt correctly the snapshot handling after this makes sense.

The reverse order may be easier: fixing the snapshot logic in the "simpler" case when extids are only involved for existing heads, then introducing the extid matching logic for arbitrary revisions.

Either way, it doesn't matter much, as I assume both will get deployed at the same time.

ardumont updated this revision to Diff 22674.Sep 15 2021, 11:08 AM

Rebase on top of D6262

swh-public-ci added a comment.Sep 15 2021, 11:09 AM

Build is green

Patch application report for D6240 (id=22674)

Could not rebase; Attempt merge onto 9be124af21...

Updating 9be124a..5fe2a9b
Fast-forward
 swh/loader/mercurial/from_disk.py            | 95 ++++++++++++++++++++--------
 swh/loader/mercurial/tests/test_from_disk.py | 19 +++++-
 2 files changed, 87 insertions(+), 27 deletions(-)
Changes applied before test
commit 5fe2a9bf77d6667b55a912b58c0f3350390d2cda
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 10 17:00:15 2021 +0200

    Add support to filter out already seen revisions across hg origins
    
    This uses the extid mapping from the storage to actually achieve this.
    
    Related to T3563

commit 139061bcc38bd5652a266b5eb0645a1492e7a5aa
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 15 10:24:48 2021 +0200

    test: Explicit that 2 visits without change ends up with no snapshot

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

Harbormaster completed remote builds in B23607: Diff 22674.Sep 15 2021, 11:10 AM
ardumont added inline comments.Sep 15 2021, 11:20 AM
swh/loader/mercurial/tests/test_from_disk.py
741

Either way, it doesn't matter much, as I assume both will get deployed at the same time.

yeah, i'm in that mind logic as well.

ardumont updated this revision to Diff 22695.Sep 15 2021, 2:59 PM

Add back dropped test by mistake (rebase messed up probably)

ardumont updated this revision to Diff 22696.Sep 15 2021, 3:00 PM

Use correct range of commits

swh-public-ci added a comment.Sep 15 2021, 3:01 PM

Build is green

Patch application report for D6240 (id=22695)

Rebasing onto 9be124af21...

Current branch diff-target is up to date.
Changes applied before test
commit 139061bcc38bd5652a266b5eb0645a1492e7a5aa
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 15 10:24:48 2021 +0200

    test: Explicit that 2 visits without change ends up with no snapshot

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

Harbormaster completed remote builds in B23628: Diff 22695.Sep 15 2021, 3:01 PM
swh-public-ci added a comment.Sep 15 2021, 3:02 PM

Build is green

Patch application report for D6240 (id=22696)

Could not rebase; Attempt merge onto 9be124af21...

Updating 9be124a..3231f2e
Fast-forward
 swh/loader/mercurial/from_disk.py            | 95 ++++++++++++++++++++--------
 swh/loader/mercurial/tests/test_from_disk.py | 77 +++++++++++++++++++++-
 2 files changed, 145 insertions(+), 27 deletions(-)
Changes applied before test
commit 3231f2ed19962275d567f7bf39127a2d0621697d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 10 17:00:15 2021 +0200

    Add support to filter out already seen revisions across hg origins
    
    This uses the extid mapping from the storage to actually achieve this.
    
    Related to T3563

commit 139061bcc38bd5652a266b5eb0645a1492e7a5aa
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 15 10:24:48 2021 +0200

    test: Explicit that 2 visits without change ends up with no snapshot

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

Harbormaster completed remote builds in B23629: Diff 22696.Sep 15 2021, 3:02 PM
ardumont updated this revision to Diff 22715.Sep 15 2021, 4:58 PM

Rebase

swh-public-ci added a comment.Sep 15 2021, 5:01 PM

Build is green

Patch application report for D6240 (id=22715)

Could not rebase; Attempt merge onto 9be124af21...

Updating 9be124a..1c6c7e5
Fast-forward
 swh/loader/mercurial/from_disk.py            | 95 ++++++++++++++++++++--------
 swh/loader/mercurial/tests/test_from_disk.py | 75 +++++++++++++++++++++-
 2 files changed, 143 insertions(+), 27 deletions(-)
Changes applied before test
commit 1c6c7e5be82df5f2419e9727d6b5f0e74e9494d9
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 10 17:00:15 2021 +0200

    Add support to filter out already seen revisions across hg origins
    
    This uses the extid mapping from the storage to actually achieve this.
    
    Related to T3563

commit bf39050051ec5c7b4c6cc7b4801079ea591cd549
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 15 10:24:48 2021 +0200

    test: Explicit that 2 visits without change ends up with no snapshot

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

Harbormaster completed remote builds in B23648: Diff 22715.Sep 15 2021, 5:01 PM
ardumont marked an inline comment as done.Sep 15 2021, 5:02 PM
ardumont updated this revision to Diff 22722.Sep 15 2021, 6:12 PM

Group into chunks prior to discuss with storage

swh-public-ci added a comment.Sep 15 2021, 6:14 PM

Build is green

Patch application report for D6240 (id=22722)

Rebasing onto bf39050051...

Current branch diff-target is up to date.
Changes applied before test
commit 4c9eb38ec516de3f65c3996d937cd3328ded6261
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 10 17:00:15 2021 +0200

    Add support to filter out already seen revisions across hg origins
    
    This uses the extid mapping from the storage to actually achieve this.
    
    Related to T3563

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

Harbormaster completed remote builds in B23655: Diff 22722.Sep 15 2021, 6:14 PM
ardumont added inline comments.Sep 16 2021, 10:12 AM
swh/loader/mercurial/from_disk.py
302

It's changed that way (from a comprehension to a for loop) because of the next step [1] (and i got lazy after the gazillionth rebase).

[1] D6268

vlorentz added a comment.Sep 16 2021, 11:15 AM

LGTM

swh/loader/mercurial/from_disk.py
345–370

why did you remove the return type?

382

seen_revs will typically be very large; does Mercurial handle this query nicely?

384–387
ardumont added inline comments.Sep 16 2021, 12:07 PM
swh/loader/mercurial/from_disk.py
345–370

a mistake ;)
it gets removed, well transformed, in the next though.

382

Well, i don't know but it was already that way before.
It just now does some more substraction now.

How can I check this?

I'm planning (to land) and check that on staging first on overall large repositories.

to land or test it in a venv, i'm not sure yet.

ardumont added inline comments.Sep 16 2021, 12:11 PM
swh/loader/mercurial/from_disk.py
345–370

Nope, it's transformed here.
I'll add it back nonetheless, thx.

ardumont added inline comments.Sep 16 2021, 12:34 PM
swh/loader/mercurial/from_disk.py
384–387

i'm not a huge fan of this writing but ok.

ardumont added inline comments.Sep 16 2021, 12:37 PM
swh/loader/mercurial/from_disk.py
384–387

Note, that's not it.
I'm not returning a list, i'm yielding stuff.

i'd prefer yield from self._new... then.

ardumont updated this revision to Diff 22744.Sep 16 2021, 1:48 PM

Adapt according to last comments

swh-public-ci added a comment.Sep 16 2021, 1:50 PM

Build is green

Patch application report for D6240 (id=22744)

Rebasing onto bf39050051...

Current branch diff-target is up to date.
Changes applied before test
commit 765cb5aefa8bb370bc1343d990ad328188a4eba5
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 10 17:00:15 2021 +0200

    Add support to filter out already seen revisions across hg origins
    
    This uses the extid mapping from the storage to actually achieve this.
    
    Related to T3563

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

Harbormaster completed remote builds in B23675: Diff 22744.Sep 16 2021, 1:50 PM
vlorentz added inline comments.Sep 16 2021, 2:13 PM
swh/loader/mercurial/from_disk.py
382

I don't know; load a large repo

ardumont added inline comments.Sep 16 2021, 2:57 PM
swh/loader/mercurial/from_disk.py
382

yes, but i thought you add another idea ;)
The venv is giving me a hard time though [1] so i'm of a mind to land and check the result.

[1]
workers are running buster.
the install requires (pip install -e .) some confluent-kafka with a version 1.6 more recent than what's proposed on debian (1.5).
Hence not working...
And it seems i'm not able to say, drop that deps.

ardumont added inline comments.Sep 16 2021, 2:59 PM
swh/loader/mercurial/from_disk.py
382

ah that's a new deps grown out of the new deps from storage on swh.counter in the 0.37 storage.
i've successfully installed my venv installing the previous version of storage.

ardumont added inline comments.Sep 16 2021, 4:21 PM
swh/loader/mercurial/from_disk.py
382

pip hell unstuck, fixed.

I've made a pass on the origin https://foss.heptapod.net/mercurial/tortoisehg/thg which qualifies
It's faster and consistent se P1167 ;)

ardumont mentioned this in T3563: Analyze and make the bitbucket ingestion faster.Sep 16 2021, 5:15 PM
This revision was not accepted when it landed; it landed in state Needs Review.Sep 17 2021, 10:05 AM
Closed by commit rDLDHG765cb5aefa8b: Add support to filter out already seen revisions across hg origins (authored by ardumont). · Explain Why
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDLDHG765cb5aefa8b: Add support to filter out already seen revisions across hg origins.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
loader/
mercurial/
98 lines91%
Loading...
tests/
58 lines100%
Loading...

Diff 22744

View Options

swh/loader/mercurial/from_disk.py

  • This file was added.
# Copyright (C) 2020 The Software Heritage developers
# See the AUTHORS file at the top-level directory of this distribution
# License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information
import json
from pathlib import Path
import sqlite3
from swh.model.identifiers import CONTENT, SWHID, parse_swhid
from swh.web.client.client import typify
class Cache:
def __init__(self, cache_dir):
seirlUnsubmitted
Done
Inline Actions

I would like this class to be more pythonic by working as a context manager. For me, the sqlite3 connection should probably happen in an __enter__() function, and closing it in an __exit__() function. I know you cannot use a with block in the Fuse operation class (a shame...), but at least it keeps the quirky aspect of this solely in the Fuse class.

seirl: I would like this class to be more pythonic by working as a context manager. For me, the…
seirlUnsubmitted
Done
Inline Actions

As discussed, we probably want to have two caches (they can probably both inherit some common behavior from an abstract base if that's helpful).

If you do go ahead with two classes, you will be able to use getitem and setitem to enable a more pythonic way of accessing this cache, like blob_cache[swhid] = X.

seirl: As discussed, we probably want to have two caches (they can probably both inherit some common…
if cache_dir == ":memory:":
self.conn = sqlite3.connect(":memory:")
else:
cache_path = Path(cache_dir, "cache.sqlite")
cache_path.parents[0].mkdir(parents=True, exist_ok=True)
self.conn = sqlite3.connect(cache_path)
self.db = self.conn.cursor()
self.db.execute("create table if not exists metadata_cache (swhid, metadata)")
self.db.execute("create table if not exists blob_cache (swhid, blob)")
self.conn.commit()
zackUnsubmitted
Done
Inline Actions

if the sqlite3 module is a well-behaved Python DB module, this could be better written with a contextmanager that commits at the end

zack: if the sqlite3 module is a well-behaved Python DB module, this could be better written with a…
seirlUnsubmitted
Done
Inline Actions

I think it's weird to keep a long-lived cursor in the class, you should probably create a new one whenever you're executing something without the conn.execute*( shortcuts. I think the class should only hold the connection as its state, not the currently active cursors.

seirl: I think it's weird to keep a long-lived cursor in the class, you should probably create a new…
def get_metadata(self, swhid: SWHID):
zackUnsubmitted
Done
Inline Actions

Please add docstring to all public methods, classes and modules. No need to be very detailed for now, oneliners would do for starters. But it's much better to have them since the beginning than having to add all of them later.

zack: Please add docstring to all public methods, classes and modules. No need to be very detailed…
self.db.execute(
"select metadata from metadata_cache where swhid=?", (str(swhid),)
)
cache = self.db.fetchone()
if cache:
metadata = json.loads(cache[0])
return typify(metadata, swhid.object_type)
def put_metadata(self, swhid: SWHID, metadata):
zackUnsubmitted
Done
Inline Actions

methods that do not return anything should be explicitly annotated with -> None, otherwise the default return value is Any (h/t @olasd )

zack: methods that do not return anything should be explicitly annotated with `-> None`, otherwise…
self.db.execute(
"insert into metadata_cache values (?, ?)",
(
str(swhid),
json.dumps(
metadata,
# Converts the typified JSON to plain str version
default=lambda x: x.object_id
if isinstance(x, SWHID)
else x.__dict__,
),
),
)
self.conn.commit()
def get_metadata_swhids(self):
self.db.execute("select swhid from metadata_cache")
swhids = self.db.fetchall()
swhids = [parse_swhid(x[0]) for x in swhids]
return swhids
def get_blob(self, swhid: SWHID):
if swhid.object_type != CONTENT:
zackUnsubmitted
Done
Inline Actions

We might want to fix this in swh-web-client, making typify optional in return values, as it's really silly to typify there and goes the other way here. Can you file a dedicated task for this?

zack: We might want to fix this in swh-web-client, making typify optional in return values, as it's…
haltodeAuthorUnsubmitted
Done
Inline Actions

Filed in: T2646.

haltode: Filed in: T2646.
raise AttributeError("Cannot retrieve blob from non-content object type")
seirlUnsubmitted
Done
Inline Actions

While this fix is here temporarily, make it less hard to read by using parentheses, it's not obvious what the "if" applies to here.

seirl: While this fix is here temporarily, make it less hard to read by using parentheses, it's not…
self.db.execute("select blob from blob_cache where swhid=?", (str(swhid),))
cache = self.db.fetchone()
if cache:
blob = cache[0]
return blob
def put_blob(self, swhid: SWHID, blob):
self.db.execute("insert into blob_cache values (?, ?)", (str(swhid), blob))
self.conn.commit()
View Options

swh/loader/mercurial/tests/test_from_disk.py

  • This file was added.
# Copyright (C) 2020 The Software Heritage developers
# See the AUTHORS file at the top-level directory of this distribution
# License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information
import json
from pathlib import Path
import sqlite3
from swh.model.identifiers import CONTENT, SWHID, parse_swhid
from swh.web.client.client import typify
class Cache:
def __init__(self, cache_dir):
seirlUnsubmitted
Done
Inline Actions

I would like this class to be more pythonic by working as a context manager. For me, the sqlite3 connection should probably happen in an __enter__() function, and closing it in an __exit__() function. I know you cannot use a with block in the Fuse operation class (a shame...), but at least it keeps the quirky aspect of this solely in the Fuse class.

seirl: I would like this class to be more pythonic by working as a context manager. For me, the…
seirlUnsubmitted
Done
Inline Actions

As discussed, we probably want to have two caches (they can probably both inherit some common behavior from an abstract base if that's helpful).

If you do go ahead with two classes, you will be able to use getitem and setitem to enable a more pythonic way of accessing this cache, like blob_cache[swhid] = X.

seirl: As discussed, we probably want to have two caches (they can probably both inherit some common…
if cache_dir == ":memory:":
self.conn = sqlite3.connect(":memory:")
else:
cache_path = Path(cache_dir, "cache.sqlite")
cache_path.parents[0].mkdir(parents=True, exist_ok=True)
self.conn = sqlite3.connect(cache_path)
self.db = self.conn.cursor()
self.db.execute("create table if not exists metadata_cache (swhid, metadata)")
self.db.execute("create table if not exists blob_cache (swhid, blob)")
self.conn.commit()
zackUnsubmitted
Done
Inline Actions

if the sqlite3 module is a well-behaved Python DB module, this could be better written with a contextmanager that commits at the end

zack: if the sqlite3 module is a well-behaved Python DB module, this could be better written with a…
seirlUnsubmitted
Done
Inline Actions

I think it's weird to keep a long-lived cursor in the class, you should probably create a new one whenever you're executing something without the conn.execute*( shortcuts. I think the class should only hold the connection as its state, not the currently active cursors.

seirl: I think it's weird to keep a long-lived cursor in the class, you should probably create a new…
def get_metadata(self, swhid: SWHID):
zackUnsubmitted
Done
Inline Actions

Please add docstring to all public methods, classes and modules. No need to be very detailed for now, oneliners would do for starters. But it's much better to have them since the beginning than having to add all of them later.

zack: Please add docstring to all public methods, classes and modules. No need to be very detailed…
self.db.execute(
"select metadata from metadata_cache where swhid=?", (str(swhid),)
)
cache = self.db.fetchone()
if cache:
metadata = json.loads(cache[0])
return typify(metadata, swhid.object_type)
def put_metadata(self, swhid: SWHID, metadata):
zackUnsubmitted
Done
Inline Actions

methods that do not return anything should be explicitly annotated with -> None, otherwise the default return value is Any (h/t @olasd )

zack: methods that do not return anything should be explicitly annotated with `-> None`, otherwise…
self.db.execute(
"insert into metadata_cache values (?, ?)",
(
str(swhid),
json.dumps(
metadata,
# Converts the typified JSON to plain str version
default=lambda x: x.object_id
if isinstance(x, SWHID)
else x.__dict__,
),
),
)
self.conn.commit()
def get_metadata_swhids(self):
self.db.execute("select swhid from metadata_cache")
swhids = self.db.fetchall()
swhids = [parse_swhid(x[0]) for x in swhids]
return swhids
def get_blob(self, swhid: SWHID):
if swhid.object_type != CONTENT:
zackUnsubmitted
Done
Inline Actions

We might want to fix this in swh-web-client, making typify optional in return values, as it's really silly to typify there and goes the other way here. Can you file a dedicated task for this?

zack: We might want to fix this in swh-web-client, making typify optional in return values, as it's…
haltodeAuthorUnsubmitted
Done
Inline Actions

Filed in: T2646.

haltode: Filed in: T2646.
raise AttributeError("Cannot retrieve blob from non-content object type")
seirlUnsubmitted
Done
Inline Actions

While this fix is here temporarily, make it less hard to read by using parentheses, it's not obvious what the "if" applies to here.

seirl: While this fix is here temporarily, make it less hard to read by using parentheses, it's not…
self.db.execute("select blob from blob_cache where swhid=?", (str(swhid),))
cache = self.db.fetchone()
if cache:
blob = cache[0]
return blob
def put_blob(self, swhid: SWHID, blob):
self.db.execute("insert into blob_cache values (?, ?)", (str(swhid), blob))
self.conn.commit()
About · Developer information · Legal