⚙ D490 Add OriginIndexer + OriginHeadIndexer.

vlorentz created this revision.Oct 4 2018, 6:26 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptOct 4 2018, 6:26 PM

Harbormaster completed remote builds in B1527: Diff 1467.Oct 4 2018, 6:26 PM

only minor issues for me, looks good otherwise

swh/indexer/indexer.py
387	should be "implements origin indexing" More generally in this file (not related to this diff but might be a chance to fix it) we use "indexation" which means something else in English. We should use "indexing" consistently
swh/indexer/origin_head.py
111	We shouldn't have a default here, as there is no sane default. If you want the list somewhere for ease of testing please put them in a REAME or use a local script.

This revision now requires changes to proceed.Oct 5 2018, 10:54 AM

Fix doc.
Index ftp.

Harbormaster completed remote builds in B1552: Diff 1494.Oct 5 2018, 2:30 PM

vlorentz marked an inline comment as done.Oct 5 2018, 2:31 PM

moranegg added inline comments.Oct 5 2018, 2:41 PM

swh/indexer/origin_head.py
44	if you are keeping prints for testing, don't mind this comment. If not, don't forget erasing them (I forget them a lot).
46	i would rename method or comment with an explicit comment saying that each origin type requires a different HEAD retrieval method but i'm being picky..
57	Here the result is without the metadata and will be kept in the DB as a pointer to the last seen revision. do you want to elaborate this by launching and copying the revision_metadata? or will it be another component? It seems a good compartmentalization. Maybe an orchestrator who uses the OriginIndexer's results to launch the RevisionIndexer with the list of revision ids?
81	I'm not a real python programmer, so this is not my place to say but the else after the except looks kinda misplaced (esthetically of course). I think it's only me, so you can completely ignore this comment.
91	is 'alias' and 'revision' the only target_type possible?
111	I used a default list with the RevisionMetatadaIndexer to test the component outside the MockStorage and didn't think to delete it. A script is a good alternative and adding a test to this or the next diff would be nice.

vlorentz marked 5 inline comments as done.Oct 5 2018, 2:48 PM

vlorentz added inline comments.

swh/indexer/origin_head.py
57	+1 for using another component
81	It's a block that is run if the `try:` block did not raise an exception. I can drop the `else:` if you find it less readable.
91	afaik, yes.
111	I'll turn these into tests.

vlorentz marked an inline comment as done.Oct 5 2018, 3:10 PM

vlorentz added inline comments.

swh/indexer/origin_head.py
91	psql does not agree: softwareheritage=> select distinct target_type from snapshot_branch; target_type ------------- content release revision alias directory I'm not sure what to do with `content` and `directory` though. I'll create an issue later.

vlorentz marked an inline comment as done.Oct 8 2018, 11:56 AM

vlorentz added inline comments.

swh/indexer/origin_head.py
57	Now that I think about it, it may affect the search between the time the OriginHead indexer runs and updates the reference, and the time the revision metadata is copied. We should probably discuss this further, outside a Diff inline comments

Add placeholders for other target types.
Start adding tests.
More tests.

Harbormaster completed remote builds in B1579: Diff 1539.Oct 8 2018, 2:40 PM

This looks good to me. Testing is also great for later iterations.

With the origin_head.py main function I have an error because origin_head_add isn't defined in my code, which is normal, waiting for the diff on the indexer storage.

I played a bit with the main function origin_head.py trying to launch the RevisionMetadataIndexer, commenting line 442 in indexer.py
which makes me more keen on keeping just the revision_id for an origin and not copy the metadata to the origin level.

swh/indexer/origin_head.py
57	I agree. I'm voting to not copy the metadata and keep only the reference (which might change quickly) The intrinsic metadata found in a revision is a more permanent object than the one we can associate with an origin.
161	I don't mind keeping the main method with the defaults here for a few iterations before deleting it.
swh/indexer/tests/test_utils.py
148 ↗	(On Diff #1539)	you should probably use a different id (even if this is a mock storage)

vlorentz added a task: T1231: Create an origin indexer that lists the most recent revision in HEAD branch.Oct 9 2018, 9:32 AM

Deduplicate tool id.
Remove default -i arguments to indexers.

Harbormaster completed remote builds in B1583: Diff 1546.Oct 9 2018, 9:37 AM

moranegg accepted this revision.Oct 9 2018, 10:42 AM

ardumont added a subscriber: ardumont.Oct 9 2018, 2:08 PM

ardumont added inline comments.

swh/indexer/indexer.py
89	you can remove the parenthesis mention now.
422	`Expected` a (type...
440	Maybe worth mentioning the origin in question in the error log.
swh/indexer/origin_head.py
27	The default configuration should be targeting a remote storage [1], defaulting to a local one as per [2] for example. [1] D372#inline-1828 [2] https://docs.softwareheritage.org/devel/getting-started.html#step-4-ingest-repositories
73	For the snapshot, possible target_type per loader: mercurial: release, revision [1] pypi: release, revision, alias (pointer to another release/revision mentioned in the snapshot) [2] svn: revision [3] [1] https://forge.softwareheritage.org/source/swh-loader-mercurial/browse/master/swh/loader/mercurial/loader.py$475-482 [2] https://forge.softwareheritage.org/source/swh-loader-pypi/browse/master/swh/loader/pypi/loader.py$239-271 [3] https://forge.softwareheritage.org/source/swh-loader-svn/browse/master/swh/loader/svn/loader.py$34-53

vlorentz marked 6 inline comments as done.Oct 9 2018, 2:17 PM

vlorentz added inline comments.

swh/indexer/origin_head.py
27	Sorry, I didn't mean to commit that.
73	What would the name of the "head" "branch" in mercurial be? "tip"?

Small fixes.

Harbormaster completed remote builds in B1595: Diff 1559.Oct 9 2018, 2:18 PM

Remove config that wasn't supposed to be committed.

Harbormaster completed remote builds in B1596: Diff 1561.Oct 9 2018, 2:19 PM

As per yesterday's chat, the result of OriginHeadIndexer should not be persistant.

Harbormaster completed remote builds in B1604: Diff 1572.Oct 10 2018, 9:08 AM

vlorentz retitled this revision from Add OriginIndexer + draft of OriginHeadIndexer. to Add OriginIndexer + OriginHeadIndexer..Oct 10 2018, 10:04 AM

ardumont added inline comments.Oct 10 2018, 11:15 AM

swh/indexer/origin_head.py
73	`default` is the head branch. `tip` is what they use to define it, i think.

zack requested changes to this revision.Oct 15 2018, 10:06 AM

zack removed a reviewer: Reviewers.

zack added inline comments.

swh/indexer/indexer.py
409	thumbs up for the detailed type annotations here
swh/indexer/origin_head.py
23	As this is going to be persisted in the DB, it's time to define some decent naming for the indexer tools. This is the "metadata indexer" so we should use it somewhere. Also, we should go hierarchically: metadata → {origin,content,revision,etc.} The fact we take the "head" revision is an implementation detail, it's just how the "origin metadata indexer" currently works. "indexer" can remain implicit, as it's what all the tools do. Bottom line: let's go for "metadata-origin".
56	as per T1257 this is no longer git-specific, so this should be `_try_get_head`
66	… for the same reason this should now be removed
70	I'm pretty sure this regex should not be here. It's a naming convention that, I guess, is also implemented by the tarball loader, right? If so it should be exposed as a class attribute by it and used from here, to avoid duplicating it around. (Unless you've very valid reasons to duplicate it here; if so they should be documented in a comment here.)
79	same for this method. (And hence maybe we do not need to expose the regex, but rather a filename parsing method from the tarball loader?)
swh/indexer/tests/test_utils.py
146 ↗	(On Diff #1572)	should be updated according to the name change suggested above

This revision now requires changes to proceed.Oct 15 2018, 10:06 AM

Better tool name (thx @zack).

Harbormaster completed remote builds in B1647: Diff 1640.Oct 15 2018, 10:25 AM

vlorentz marked an inline comment as done.Oct 15 2018, 10:25 AM

vlorentz added inline comments.

swh/indexer/origin_head.py
70	As far as I understand, the tarball loader does not try to parse the file name; it only downloads a tarball from an URL, extracts it, and forwards it to the dir loader (which doesn't seem to parse names either).