Please as mentioned already, use CamelCase as defined everywhere else (same goes for other classes).

I realize that it's not necessary true.
The offset should be part of the find_date implementation...

The docstring is below the variable definition.

compare_field = None
"""Field used to identify if the p..."""

Yes, you are correct, actually, it is already the part of find_date, but I realized it now

Can you please check once for this method that docstring appropriate and correctly explains what is done in the function.
I am not sure about this method that the thing I stated in docstring is actually done by this method

ping ;)

Those should be docstrings, not values of the variables.

loader_name = None
"""Package manager name"""
class_name = None
"""Loader class name"""
etc...

If those are for the configuration file to load, we are moving away from this (even though, that's not apparent in the code).
Please remove.

We use the environment variable stanza (i explained to you in irc) to deploy those, be that in the docker-dev or the production environment.

It's the method we use everywhere to compute hashes on a file.

This method does 2 things, given 1 response which holds a raw file to some request:

• compute the hashes (sha1, sha1_git, sha256, blake2s) of the file
• write the raw file in filepath

Implementation wise, this does this in one round-trip.

It then returns the hashes (dict) of the file.

IIRC, the writing part was done in a cache (on disk).

I don't understand that method.

There is a HEAD for every branch that is created by the loader.
In NPM loader this head branch is predicted by the latest version ie the latest version of the packages is the head of the branch.

Now, this is possible when the loader with NPM to determine the latest release because the version of the package is provided in the metadata.

But there could be some package manager(in fact many) which do not provide the version metadata or some metadata which could be used to determine the latest release to determine the HEAD.

So I am not sure how to make properly identify the head of the branch hence for testing purpose I have done if True:

One possible way out could be we just leave this method to be made in all the loaders separately(as done in convert_to_standard_format().

Is there any better approach to solve this problem?

And is it necessary to have HEAD for the branch?

Improve docstring

Same problem of unavailability of metadata as mentioned in the inline comment find_head().
Same solution:
One possible way out could be we just leave this method to be made in all the loaders separately(as done in convert_to_standard_format().

Is there any better approach to solve this problem?

I don't really know where to define that part without duplicating... (it's already in the docker-dev for other loaders and in prod since we need it to work there ;)
So maybe, let's not spend too much time on this documentation part for now.

Also, i'd need to check but the additional_config is also something to deprecate (so to remove here)...
While that documents a bit what's the expectations around config is.
That's not something that we want to keep.

Rationale: if it's not maintained anymore, it's dead code, so that must go away.

Changed known_versions=None to known_versions={} because if known_versions is None then it would create a problem later in code

Change the function name from known_versions() to known_version() because there was a class variable self.known_versions.

previously function definition was def check_file(self, length, filepath): but it was called as self.check_file(filepath, length) which lead to assigning of filepath value to length variable. Fixed this bug

I am not a big fan of using inheritance in derived package loaders implementation (CRAN, GNU, ...) to declare the package tarballs retrieval behavior.
I would rather use a single class, named PackageDownloader for instance, that will internally be used by the PackageLoader class through composition instead.

Derived classes implementing real packages loader (GNU, CRAN, ..) should only inherit from the PackageLoader class and reimplement some hook methods to override the default behavior.

I need to further analyze what is implemented in that file before suggesting improvements.

How about renaming that class to PackageLoader ? This feels more explicit about what it is intended to do.

Also, I would rename the folder containing these new source files from base to package.
Maybe this could also be moved in a new dedicated swh-loader-package repository, but let's
keep it there at the moment.

From my point of view, this method is useless and could be removed if we slightly update the package listers implementation (GNU, CRAN, npm, PyPI, ...). Those listers should provide the following parameters to the loader: name, origin_url and tarballs.

The correct signature of this method is:

def prepare_origin_visit(self, *args, **kwargs):

The loader will fail to execute when executed by a celery worker otherwise (this is what we used in production to schedule and execute the loading of software origins in the archive)..

If we remove the convert_to_standard_format method and ensure consistent inputs provided by the listers to that loader, this could be changed in:

self.package_details = kwargs

Same here, correct signature is:

def prepare(self, *args, **kwargs):

I would rather rename it to get_known_versions, the plural use is important here

new_versions seems a better name here

Please capitalize the first letter of that class name.

I am not a big fan of using inheritance in derived package loaders implementation (CRAN, GNU, ...) to declare the package tarballs retrieval behavior.

Yes, thank you. I did not find the right way to explicit that.

We must try and keep separated what's logically separated (artefacts retrieval, ingestion, etc...).

I would rather use a single class, named PackageDownloader for instance, that will internally be used by the PackageLoader class through composition instead.

Yes, that's what i call a collaborator.
PackageDownloader is a collaborator of the PackageLoader.
It's what's being done in the pypi and npm loader IIRC with ArchiveFetcher (or something).

How about renaming that class to PackageLoader ? This feels more explicit about what it is intended to do.

I like it better indeed!

Also, I would rename the folder containing these new source files from base to package.

I asked to move those back in core.
But your proposal is way better.
Please do so in the end ;)

Maybe this could also be moved in a new dedicated swh-loader-package repository, but let's

keep it there at the moment.

That's what we (olasd, nahimilega and me) agreed on from the start.
First let's keep it here.
Then, when we are mostly satisfied, we'll move it.

I agree again.
If we converge with a use case, we can change the lister implementation to unify their output (scheduled tasks).

Note that it means a migration from the existing recurring tasks in the scheduler db (out of scope from this task ;)

i agree!

Also, we'll also need to be able to build "release"?

loader-deposit for one will need it.

I am again not sure we should do this because, as once olasd told that we parallelize the loading task. Hence we should not keep the API call(to get metadata) step in the listers.
For package managers for a huge amount of packages, lister will take up a lot of time to complete whereas this could be done in the loader and make it fast using parallelization.

I don't think that's a good idea. convert_to_standard_format is provided for loaders where we need to make an API call to get the metadata(eg pypi).

Something rings a bell indeed.
Moving the metadata retrieval done lister side to the loader-side (as the debian loader does).
But i don't remember the details of that exchange (most probably, this is detailed in the task).

Your sentences are a bit unclear.

I am again not sure we should do this because, as once olasd told that we parallelize the loading task. Hence we should not keep the API call(to get metadata) step in the listers.

I'm not sure i see the causality between the parallelize the loading-task and moving the api call to solve metadata loader side.

For package managers for a huge amount of packages, lister will take up a lot of time to complete whereas this could be done in the loader and make it fast using parallelization.

Same here.
Be that loaders or listers, they all doing their job concurrently.

That a lister takes some time to list is not per say a problem.
Because surely, it does not exceed the worst case we already have on loaders.
As of today, given certain origins, some loader-git/mercurial/svn takes forever to finish...

In any case, if this method is the one that solves the metadata loading, it's not clear from its name.
I now realize i must have skim through though as i see the docstring tries to explain this.
Still, i think the name should be improved upon to explicit that.

I'm not sure I see the causality between the parallelise the loading-task and moving the API call to solve metadata loader side.

My point was, when I was making packagist lister, in the first pass, I kept the metadata retrieval step (it would send an api request for each package to get metadata) in the lister itself. While testing it, on my pc, it took like 3 hours to list only about 5 % of the total packages.

Now if this task is done in loading. The lister will pass the metadata URL of the package to the loader. And we will have a separate loading task for each package. If we can parallelise the task(i.e. run many loading tasks at the same time), then we could make metadata request for many packages at the same time, and it will make the whole process fast.
As compared it lister where first all the metadata calls are made, the loading task is created.
(I am emphasising on metadata request part so much because while testing listers, I felt this step is like a bottleneck. it is slow)

One more point it will make the whole system more robust. As let us suppose that there are exceptions in the metadata API for one or two package, like some field is not present or was present at the time lister was developed now removed.
Now if we make that metadata request in lister, then it will encounter an error, and the whole process might stop(although this possibility could be removed if the lister code is robust and written to handle these error).
But if the metadata request is made in the loader, then the loading task of that particular package will fail. And everything will still work fine.

Hope it is a bit clear what I am trying to say ;)

In any case, if this method is the one that solves the metadata loading, it's not clear from its name.

I will try to make it more explicit, but actually, the task of that method is to make the metadata request(if needed) and convert the output coming from lister to a standard format which is used in the loader. The standard format is the format as shown in docstring.

@nahimilega , I totally agree with you that the metadata extraction for a package must be handled loader-side. This calls indeed for a dedicated method to perform that task in the PackageLoader class that derived ones must reimplement.

But keep in mind that metadata extraction must be performed for each version of a package and that most of the times there is no need to perform an API call to a remote service as package content usually provide a file filled with these information that can be easily parsed (package.json, PKG-INFO, ...).

So trying to extract metadata before iterating on each version of a package to load into the archive is not the right solution.

I was also confused by the naming of the method, convert_to_standard_format is not reallly explicit about its role. As @ardumont explained, every processiong step for loading the different versions of a package into the archive should be clearly identified with enough granularity to cover all package managers available in the wild. This implies a lot of brainstorming to do it well and define an API that will be easily usable and maintainable ;-)

Let's continue that work next week !

But keep in mind that metadata extraction must be performed for each version of a package.

But for the package managers where we don't even know the versions(eg pypi) and we need to make a API call to get a list of package version, in that case, convert_to_standard_format can be used.

And for extraction of metadata from the file provided(package.json, PKG-INFO, ...) extract_metadata method can be used. It iterated on each version of a package.