Page MenuHomeSoftware Heritage
Differential D2461

add Python client for the archive WEB API
AbandonedPublic
Actions

Authored by zack on Dec 16 2019, 4:50 PM.

Details

Reviewers
None
Group Reviewers
Reviewers
Summary

not meant to land in snippets, but it's not clear where else it should land, so
I'm posting here for review purposes, we can determine where to land it later

Related: T2279

Diff Detail

Repository
rDSNIP Code snippets
Branch
webclient
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 9812
Build 14489: arc lint + arc unit

Event Timeline

zack created this revision.Dec 16 2019, 4:50 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptDec 16 2019, 4:50 PM
Harbormaster completed remote builds in B9793: Diff 8706.Dec 16 2019, 4:50 PM
vlorentz added a subscriber: vlorentz.Dec 16 2019, 5:02 PM

Looks good overall, but I have a lot of nitpicks

zack/webclient.py
105–127

You could get rid of all these elif switches by splitting the typify function, and removing jsonify.

136

Wrong name, as it's reading from JSON, instead of producing JSON.

205–214

Can be shorter:

functions = {
    SNAPSHOT: self.snapshot,
    REVISION: self.revision,
    # ...
}
return functions[pid_.object_type](pid_)

(and it would check the type is known)

225–234

same

298–330

What about generating a list of branches instead? That's what callers of this function will have to do one way or an other, anyway.

418–419

yield from r.iter_content(chunk_size=None, decode_unicode=False)

zack updated this revision to Diff 8724.Dec 17 2019, 10:18 AM
  • simplify code and streamline returned snapshot types
Harbormaster completed remote builds in B9811: Diff 8724.Dec 17 2019, 10:18 AM
zack updated this revision to Diff 8725.Dec 17 2019, 10:25 AM
zack marked 4 inline comments as done.
  • webclient: add missing error checking
Harbormaster completed remote builds in B9812: Diff 8725.Dec 17 2019, 10:25 AM
zack marked an inline comment as done.Dec 17 2019, 10:25 AM
zack added inline comments.
zack/webclient.py
105–127

I don't get what you mean about typify here.

If you want to split into different typify_foo functions and a dictionary based dispatched, I disagree, as some will be oneliners and then we'll have to catch the KeyError in there to reraise a ValueError. All in all, the code will be longer.

If it's something else, can you explain?

136

jsonify() was actually producing json, with .json() from the response object, and returning it as Python data.
But I got rid of it anyway as it was overkill to have a function just for that.

225–234

I haven't done dictionary based dispatching for the iter() case, because yield is syntactically invalid within lambda, so I'd have to use def-s instead for a bunch of oneliners, and that would make the code longer. (I've added the fall through else to check for errors which was missing though.

298–330

Good idea, done.

I note that this is now inconsistent with directory, which does return the parent directory id in each entry, but it's an inconsistency that is derived from the API itself, that should probably be fixed there with 2.0 anyway.

vlorentz added inline comments.Dec 17 2019, 11:01 AM
zack/webclient.py
105–127

You don't need dictionary dispatch, because all lines calling typify have the value of obj_type defined statically; so they might as well call typify_* functions directly.

douardda added a subscriber: douardda.Jan 9 2020, 10:57 AM

IMHO this should be a 'standalone' repo/project. I mean it should be possible to pip install it at least.

zack added a comment.Jan 9 2020, 11:02 AM
In D2461#59540, @douardda wrote:

IMHO this should be a 'standalone' repo/project. I mean it should be possible to pip install it at least.

as mentioned above :-)

not meant to land in snippets, but it's not clear where else it should land, so
I'm posting here for review purposes, we can determine where to land it later

so, yeah, I agree!, but I'm not sure where…

zack mentioned this in T2279: Python client for the Web API.Feb 13 2020, 2:37 PM
zack edited the summary of this revision. (Show Details)
zack mentioned this in D2657: code scanner prototype.Feb 27 2020, 4:28 PM
lewo added a subscriber: lewo.Mar 11 2020, 5:01 PM
zack mentioned this in D2835: [WIP] client.py: add Python client for the Web API.Mar 16 2020, 4:10 PM
zack abandoned this revision.Mar 16 2020, 4:10 PM

abandoned in favor of D2835

Revision Contents
Changeset List

PathSize
zack/
412 lines

Diff 8725

View Options

zack/webclient.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