Page MenuHomeSoftware Heritage
Differential D1482 Diff 4872 swh/lister/gnu/lister.py

Changeset View

Standalone View

View Options

swh/lister/gnu/lister.py

  • This file was added.
# Copyright (C) 2018 the Software Heritage developers
ardumontUnsubmitted
Done
Inline Actions

copyright header needs an update.
Since it's inspired from the tar-loader, it'd be 2018-2019.

ardumont: copyright header needs an update. Since it's inspired from the tar-loader, it'd be 2018-2019.
# License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information
import random
import gzip
import json
import os
import requests
from urllib.parse import urlparse
from .models import GNUModel
from swh.scheduler import utils
from swh.lister.core.simple_lister import SimpleLister
from swh.model.hashutil import MultiHash, HASH_BLOCK_SIZE
class LocalResponse:
"""Local Response class with iter_content api
"""
def __init__(self, path):
self.path = path
ardumontUnsubmitted
Done
Inline Actions

Specify the variable's goal.

Something like, please improve as you see fit:

Dict of key the project name (/id/url?), value the associated list of tarballs to ingest from the gnu mirror
ardumont: Specify the variable's goal. Something like, please improve as you see fit: ``` Dict of key…
def iter_content(self, chunk_size=None):
with open(self.path, 'rb') as f:
for chunk in f:
yield chunk
vlorentzUnsubmitted
Done
Inline Actions

This is going to iter over bytes, not chunks; and it will be very inefficient.

Use this instead:

with open(self.path, 'rb') as f:
    while True:
        chunk = f.read(chunk_size)
        if not chunk:
            break
        yield chunk
vlorentz: This is going to iter over bytes, not chunks; and it will be very inefficient. Use this…
nahimilegaAuthorUnsubmitted
Done
Inline Actions

I copied this class from loader-tar, hence I assume we also change this in the loader-tar class to make it more efficient.

nahimilega: I copied this class from loader-tar, hence I assume we also change this in the loader-tar class…
ardumontUnsubmitted
Done
Inline Actions

I'll update the loader-tar with this.

ardumont: I'll update the loader-tar with this.
class ArchiveFetcher:
"""Http/Local client in charge of downloading archives from a
ardumontUnsubmitted
Done
Inline Actions

tarballs here as well.
That ends up in the future loader part.

ardumont: `tarballs` here as well. That ends up in the `future` loader part.
ardumontUnsubmitted
Done
Inline Actions

?

ardumont: ?
nahimilegaAuthorUnsubmitted
Done
Inline Actions

The list of tarballs is stored as the json string format in the model and needs to be converted back to list of dictionary before using it to create a loading task

nahimilega: The list of tarballs is stored as the json string format in the model and needs to be converted…
ardumontUnsubmitted
Done
Inline Actions

But why do we need the list of tarballs in the model?

I'd expect we compute it and pass it along to the loader.
But we do not need to keep it in the lister's model.

ardumont: But why do we need the list of tarballs in the model? I'd expect we compute it and pass it…
nahimilegaAuthorUnsubmitted
Done
Inline Actions

Please correct me if I am wrong somewhere -
To pass the list of tarballs to the loader we need to pass it in the loading task created by the.

To pass the list of tarballs in the loading task we need it so that it can be accessed in the task_dict() function.

To access the list of tarballs in the task_dict() we need to ensure it is present in the model.

nahimilega: Please correct me if I am wrong somewhere - To pass the list of tarballs to the loader we need…
ardumontUnsubmitted
Done
Inline Actions

Please correct me if I am wrong somewhere -
To pass the list of tarballs to the loader we need to pass it in the loading task created by the.

I'm missing the end of the sentence.

To pass the list of tarballs in the loading task we need it so that it can be accessed in the task_dict() function.
To access the list of tarballs in the task_dict() we need to ensure it is present in the model.

I'm not sure yet. I'd need to dig in the code to make sure.

All I know is that it should not be a requisite to store the computed data (needed for the loader) in the lister model.
So if the actual code requires it, it feels wrong to me.

ardumont: > Please correct me if I am wrong somewhere - > To pass the list of tarballs to the loader we…
nahimilegaAuthorUnsubmitted
Done
Inline Actions

Please correct me if I am wrong somewhere -
To pass the list of tarballs to the loader we need to pass it in the loading task created by the.

I'm missing the end of the sentence.

Sorry for that, it is "To pass the list of tarballs to the loader we need to pass it in the loading task created."

I'm not sure yet. I'd need to dig in the code to make sure.

All I know is that it should not be a requisite to store the computed data (needed for the loader) in the lister model.

I tried to do what I thought would work fine, and I am not able to come up with another way to send the tarballs of loader. Can you please look up suggest some better way.

nahimilega: > >Please correct me if I am wrong somewhere - >> To pass the list of tarballs to the loader we…
nahimilegaAuthorUnsubmitted
Done
Inline Actions

One way to avoid including tarballs in model is to make a variable instance of class named tarballs (like LISTER_NAME or TREE_URL), which would countain all the tarballs of each package and can be accessed from task_dict() function
Hence will eliminate the need of adding tarballs in model.

nahimilega: One way to avoid including tarballs in model is to make a variable instance of class named…
ardumontUnsubmitted
Done
Inline Actions

I'd expect we compute it and pass it along to the loader.

I mean that through the scheduler (so that's what the task_dict method here is all about).

ardumont: > I'd expect we compute it and pass it along to the loader. I mean that through the scheduler…
remote/local server.
Args:
temp_directory (str): Path to the temporary disk location used
for downloading the release artifacts
"""
def __init__(self, temp_directory=None):
self.temp_directory = os.getcwd()
self.session = requests.session()
self.params = {
'headers': {
'User-Agent': 'Software Heritage Tar Loader (%s)' % (
ardumontUnsubmitted
Done
Inline Actions

It's not the tar loader client, Software Heritage Lister

ardumont: It's not the tar loader client, `Software Heritage Lister`
'devl'
vlorentzUnsubmitted
Done
Inline Actions

Why use a string substitution here?

vlorentz: Why use a string substitution here?
)
}
}
def download(self, url):
"""Download the remote tarball url locally.
Args:
url (str): Url (file or http*)
ardumontUnsubmitted
Done
Inline Actions

Drop the intermediary variable.
Also, if the get_file is only used here, you can inline directly its implementation here.
That avoids indirection.

ardumont: Drop the intermediary variable. Also, if the `get_file` is only used here, you can inline…
nahimilegaAuthorUnsubmitted
Done
Inline Actions

I did it this way because if I remove safely_issue_request() method then I have to override the whole run() method from SimpleLister class just to change one line(ie the top line where safely_issue_request() is called), unnecessary increase in code.

Shall I keep it the way it is now or override the whole run() method?

nahimilega: I did it this way because if I remove safely_issue_request() method then I have to override the…
ardumontUnsubmitted
Done
Inline Actions

Ok then.
Let's keep that.

I don't like doing that, altering code for tests, but i acknowledge we need to for now.
We need to rework lister base code and tests anyhow, so we'll see then.

ardumont: Ok then. Let's keep that. I don't like doing that, altering code for tests, but i acknowledge…
Raises:
ValueError in case of failing to query
Returns:
Tuple of local (filepath, hashes of filepath)
"""
url_parsed = urlparse(url)
if url_parsed.scheme == 'file':
path = url_parsed.path
response = LocalResponse(path)
length = os.path.getsize(path)
else:
response = self.session.get(url, **self.params, stream=True)
if response.status_code != 200:
raise ValueError("Fail to query '%s'. Reason: %s" % (
url, response.status_code))
ardumontUnsubmitted
Done
Inline Actions

'tarballs' is enough.

ardumont: 'tarballs' is enough.
length = int(response.headers['content-length'])
filepath = os.path.join(self.temp_directory, os.path.basename(url))
h = MultiHash(length=length)
with open(filepath, 'wb') as f:
for chunk in response.iter_content(chunk_size=HASH_BLOCK_SIZE):
h.update(chunk)
f.write(chunk)
actual_length = os.path.getsize(filepath)
if length != actual_length:
raise ValueError('Error when checking size: %s != %s' % (
length, actual_length))
return filepath
ardumontUnsubmitted
Done
Inline Actions

Also, i guess, we could push this code from the loader-tar to the core if that needs to be reused.
That'd preferable than duplicating it.

ardumont: Also, i guess, we could push this code from the loader-tar to the core if that needs to be…
nahimilegaAuthorUnsubmitted
Done
Inline Actions

It would be great if loader-tar can be shifted to the core. That would reduce the code to less than half

nahimilega: It would be great if loader-tar can be shifted to the core. That would reduce the code to less…
ardumontUnsubmitted
Done
Inline Actions

In the end, i think it'd be best to use simply use request to download that file (which is already a module dependency).
And not duplicate that code.

There has been discussion to refactor the distribution/package loader and that code will probably disappear (so no point in pushing it to core).

ardumont: In the end, i think it'd be best to use simply use `request` to download that file (which is…
class GNULister(SimpleLister, ArchiveFetcher):
MODEL = GNUModel
LISTER_NAME = 'gnu'
FILE = 'https://ftp.gnu.org/tree.json.gz'
vlorentzUnsubmitted
Done
Inline Actions

That's an URL, not a file. And the constant's name should tell what is at that URL. (eg. TREE_URL)

vlorentz: That's an URL, not a file. And the constant's name should tell what is at that URL. (eg.
def __init__(self, override_config=None):
SimpleLister.__init__(self, override_config=override_config)
ArchiveFetcher.__init__(self, override_config=override_config)
def task_dict(self, origin_type, origin_url, **kwargs):
"""(Override)
Return task format dict
ardumontUnsubmitted
Done
Inline Actions

tarballs =

ardumont: `tarballs =`
This is overridden from the lister_base as more information is
needed for the ingestion task creation.
"""
_type = 'origin-update-%s' % origin_type
anlambertUnsubmitted
Done
Inline Actions

FYI, we are currently renaming the scheduler task names to something more rational (see T1508).
This should be changed to:

_type = 'load-%s' % origin_type
anlambert: FYI, we are currently renaming the scheduler task names to something more rational (see T1508).
_policy = 'recurring'
project_name = kwargs.get('name')
project_metadata_url = kwargs.get('html_url')
return utils.create_task_dict(
_type, _policy, project_name, origin_url,
project_metadata_url=project_metadata_url)
ardumontUnsubmitted
Done
Inline Actions

This needs to be changed to match what needs the loader-tar to ingest the tarball [1]

[1] https://forge.softwareheritage.org/source/swh-loader-tar/browse/master/swh/loader/tar/tasks.py$12

ardumont: This needs to be changed to match what needs the loader-tar to ingest the tarball [1] [1]…
nahimilegaAuthorUnsubmitted
Done
Inline Actions

As this lister is listing all the repos in gnu, mirror and old-gnu directory, there are some files in the listed repos that are not tarball and hence are not required to be loaded, hence I thought of creating a separate gnu loader which would remove useless files and then load.

nahimilega: As this lister is listing all the repos in gnu, mirror and old-gnu directory, there are some…
nahimilegaAuthorUnsubmitted
Done
Inline Actions

So I wish to make a new lister then also I have to match this according to loader-tar?

nahimilega: So I wish to make a new lister then also I have to match this according to loader-tar?
ardumontUnsubmitted
Done
Inline Actions

As this lister is listing all the repos in gnu, mirror and old-gnu directory, there are some files in the listed repos that are not tarball and hence are not required to be loaded, hence I thought of creating a separate gnu loader which would remove useless files and then load.

Discussing with @anlambert (no task yet), we entertained the idea of having a dedicated gnu loader.
It would mostly behave the same way as the npm/pypi loader (extract metadata from a metadata file, ingest the tarballs and reference them in the snapshot, result of the loading visit for that origin).

So that'd go in the direction of a dedicated loader as you mentioned.

For the files that are missing or files that are not tarballs, the lister simply needs to not list them (thus not passing them along to the loader).

So I wish to make a new lister then also I have to match this according to loader-tar?

I have trouble understanding the sentence.
No need for a new lister.

ardumont: > As this lister is listing all the repos in gnu, mirror and old-gnu directory, there are some…
nahimilegaAuthorUnsubmitted
Done
Inline Actions

Sorry, there was a typo in this sentence, it should be
So I wish to make a new loader then also I have to match this according to loader-tar?

nahimilega: Sorry, there was a typo in this sentence, it should be So I wish to make a new loader then…
ardumontUnsubmitted
Done
Inline Actions

Have a look at [1], this should be simplified to create a task whose arguments are a list of dict.
The origin becomes the root directory and the tarballs to ingest are the listed tarballs from that root directory.
In the end the difficulty of that lister becomes to determine which root directory are worth listing (i'd say elect the ones having tarballs).

Dict of key values:

  • archive: url targetting a tarball from a directory to retrieve from the gnu archive (and ingest in swh archive)
  • date: the modification time the archive got uploaded to the gnu.org mirror (which should be used somehow, probably as the revision date). Initially it was for the loading visit but no more (as we are aiming at loading a

Later, the tarball loader should be adapted (again according to [1]) to ingest those tarballs and make the appropriate snapshot (result of the one loading visit).

[1] T1389#31790

ardumont: Have a look at [1], this should be simplified to create a task whose arguments are a list of…
def download_file(self):
'''
Downloads tree.json file and returns its location
Returns
File path of the downloaded file
'''
ardumontUnsubmitted
Done
Inline Actions

Please, remove the print statement

ardumont: Please, remove the print statement
file_path, hash_dict = self.download(self.FILE)
return file_path
ardumontUnsubmitted
Done
Inline Actions

Like i said earlier, change the download implementation to use the requeststool.
Inline directly the code here, no need for an indirection.

ardumont: Like i said earlier, change the download implementation to use the `requests`tool. Inline…
def read_downloaded_file(self, file_path):
'''
Reads the downloaded file content and convert it into json format
Returns
ardumontUnsubmitted
Done
Inline Actions

Are you sure you need to do that?

ardumont: Are you sure you need to do that?
nahimilegaAuthorUnsubmitted
Done
Inline Actions

There is no datatype to store a list of dictionary in Postgres, hence either we make a separate table for this, or as it is a list of dictionary ,we can convert it into json string and store it in string datatype in database, and convert it back to list of dictionary when needed. I used the JSON method over other database as it is easy to write and understand.

nahimilega: There is no datatype to store a list of dictionary in Postgres, hence either we make a separate…
ardumontUnsubmitted
Done
Inline Actions

So checking the base code of the simple lister class (ingest_data).
The tarball key should be present at the moment of the task_dict creation for the scheduler.
So i stand by my initial statement, we should not have to transform back and forth in json the tarball dict.
It should stay as is and be out of the lister model.

ardumont: So checking the base code of the simple lister class (`ingest_data`). The `tarball` key should…
File content in json format
'''
with gzip.GzipFile(file_path, 'r') as fin:
response = json.loads(fin.read().decode('utf-8'))
return response
def safely_issue_request(self, identifier):
'''(Override)Make network request with to download the file which
has file structure of the GNU website.
Args:
identifier: resource identifier
Returns:
ardumontUnsubmitted
Done
Inline Actions

Are those override really necessary (i don't remember)?

ardumont: Are those override really necessary (i don't remember)?
nahimilegaAuthorUnsubmitted
Done
Inline Actions

Yes, they are necessary, they are mentioned as abc.abstract attribute in the simple lister class

nahimilega: Yes, they are necessary, they are mentioned as abc.abstract attribute in the simple lister…
ardumontUnsubmitted
Done
Inline Actions

You should remove those decorators from the base class.
It's not used here and makes the code immediatly harder to read and will make it harder to maintain later.

You should then remove those unused definitions (that might also simplify the cran lister diff).

ardumont: You should remove those decorators from the base class. It's not used here and makes the code…
nahimilegaAuthorUnsubmitted
Done
Inline Actions

I created D1566 to address this issue

nahimilega: I created D1566 to address this issue
server response
'''
ardumontUnsubmitted
Done
Inline Actions

Simplify the name to find_tarballs.

Improve the docstring to clarify this is done per origin/package url.

ardumont: Simplify the name to `find_tarballs`. Improve the docstring to clarify this is done per…
file_path = self.download_file()
response = self.read_downloaded_file(file_path)
ardumontUnsubmitted
Done
Inline Actions

tarball`s`

ardumont: tarball`s`
return response
def list_packages(self, response):
"""(Override) List the actual gnu origins with their names and
ardumontUnsubmitted
Done
Inline Actions

corresponding

ardumont: corresponding
time last updated from the response.
"""
response = clean_up_response(response)
_packages = []
vlorentzUnsubmitted
Done
Inline Actions

Why the underscore prefix?

vlorentz: Why the underscore prefix?
nahimilegaAuthorUnsubmitted
Done
Inline Actions

I took inspiration from PyPI lister, there it had underscore prefix hence I opted to follow the same style. Shall I change it?

nahimilega: I took inspiration from PyPI lister, there it had underscore prefix hence I opted to follow the…
ardumontUnsubmitted
Done
Inline Actions

You can drop the _, it was to be read as a private variable. It seems to be unneeded, so simply drop it ;)

ardumont: You can drop the `_`, it was to be read as a private variable. It seems to be unneeded, so…
for directory in response:
content = directory['contents']
for repo in content:
if repo['type'] == 'directory':
repo_details = {
'name': repo['name'],
'url': self._compute_urls(directory['name'],
repo['name']),
'time_modified': repo['time']
}
_packages.append(repo_details)
random.shuffle(_packages)
return _packages
ardumontUnsubmitted
Done
Inline Actions

tarballs

ardumont: `tarballs`
ardumontUnsubmitted
Done
Inline Actions

please, rename to a simpler name.

ardumont: please, rename to a simpler name.
def _compute_urls(self, dir_name, package_name):
vlorentzUnsubmitted
Done
Inline Actions

url, not urls. And the name should tell what it actually does. (eg. _get_project_url)

vlorentz: `url`, not `urls`. And the name should tell what it actually does. (eg. `_get_project_url`)
ardumontUnsubmitted
Done
Inline Actions

Declare variable for the filetype and filename to reuse within the block.
That's clearer and more concise.

ardumont: Declare variable for the filetype and filename to reuse within the block. That's clearer and…
"""Returns project_url
nahimilegaAuthorUnsubmitted
Done
Inline Actions

I am not sure this is a good method to find if the file is a tarball, can you please suggest some improvements

nahimilega: I am not sure this is a good method to find if the file is a tarball, can you please suggest…
ardumontUnsubmitted
Done
Inline Actions

That'd be simpler to read.

filename = single_file['name']
".tar" in filename or ".zip" in filename
ardumont: That'd be simpler to read. ``` filename = single_file['name'] ".tar" in filename or ".zip" in…
nahimilegaAuthorUnsubmitted
Done
Inline Actions

If we do this, then it will also include files with extension tar.gz.sig. I don't think we need to add those files as they do not contain any code.

nahimilega: If we do this, then it will also include files with extension tar.gz.sig. I don't think we need…
ardumontUnsubmitted
Done
Inline Actions

Indeed, that's inconvenient.

I guess gnu's manifest (tree.json.gz) is not moving a lot so the way forward would be to analyze and compulse the list of possible extension files from that manifest (take a snapshot of today would be enough).
While possibly tedious, it'd be the right way to determine the right implementation here.

Could you try and investigate in that direction?
(and update the task with such analysis result)

Thanks

ardumont: Indeed, that's inconvenient. I guess gnu's manifest (tree.json.gz) is not moving a lot so the…
nahimilegaAuthorUnsubmitted
Done
Inline Actions

In my view, the best method to check for tarball would be to break the filename on " . " and check if the word between last and second last " . " is "tar" or not?. If it is "tar" then the file is useful, else the file does not have source code.

nahimilega: In my view, the best method to check for tarball would be to break the filename on " . " and…
"""
return 'https://ftp.gnu.org/%s/%s/' % (dir_name, package_name)
nahimilegaAuthorUnsubmitted
Done
Inline Actions

I have used the function here, sorry in the previous revision I forgot to call the function.

And sorry for the silly mistake with the spelling of extension, I will make sure to check for spelling before submitting the diff.

nahimilega: I have used the function here, sorry in the previous revision I forgot to call the function.
ardumontUnsubmitted
Done
Inline Actions

No worries, that happens.

ardumont: No worries, that happens.
def get_model_from_repo(self, repo):
"""(Override) Transform from repository representation to model
"""
return {
'uid': repo['name'],
'name': repo['name'],
'full_name': repo['name'],
'html_url': repo['url'],
ardumontUnsubmitted
Done
Inline Actions

You should add unit tests on that function.

ardumont: You should add unit tests on that function.
'origin_url': repo['url'],
'time_last_upated': repo['time_modified'],
'origin_type': 'gnu',
'description': None,
ardumontUnsubmitted
Done
Inline Actions

filter_directories sounds clearer enough.

ardumont: `filter_directories` sounds clearer enough.
}
ardumontUnsubmitted
Done
Inline Actions

That's not really what happens here.
You filter to some initial folders with names (gnu, mirrors, old-gnu).
You do not really check for tarball existence, you assume (probably true) those folders contains tarballs ;)

ardumont: That's not really what happens here. You filter to some initial folders with names (gnu…
ardumontUnsubmitted
Done
Inline Actions

docstring update to:
"Keep only gnu and old-gnu folders from JSON."

ardumont: docstring update to: "Keep only `gnu` and `old-gnu` folders from JSON."
def transport_response_simplified(self, response):
"""(Override) Transform response to list for model manipulation
"""
return [self.get_model_from_repo(repo) for repo in response]
ardumontUnsubmitted
Done
Inline Actions

Same clean_up_response should be:

  • renamed to explicit what it does
  • unit tested
ardumont: Same `clean_up_response` should be: - renamed to explicit what it does - unit tested
ardumontUnsubmitted
Done
Inline Actions

extension here as well.

Please can you check the typo is fixed everywhere?

ardumont: extension here as well. Please can you check the typo is fixed everywhere?
def clean_up_response(response):
final_response = []
file_system = response[0]['content']
for directory in file_system:
for name in ('gnu', 'mirrors', 'old-gnu'):
if directory['name'] == name:
vlorentzUnsubmitted
Done
Inline Actions

nitpick: this is equivalent to:

if directory['name'] in ('gnu', 'mirrors', 'old-gnu'):
vlorentz: nitpick: this is equivalent to: ``` if directory['name'] in ('gnu', 'mirrors', 'old-gnu'): ```
ardumontUnsubmitted
Done
Inline Actions

extension

Although, i do not see where that function is used.
Did you use it to analyze the listing you made in the task?

ardumont: `extension` Although, i do not see where that function is used. Did you use it to analyze the…
final_response.append(directory)
return final_response
ardumontUnsubmitted
Done
Inline Actions

Check for file extension. Accepts file extension with .zip or .tar.x (x could be anything).

ardumont: Check for file extension. Accepts file extension with `.zip` or `.tar.x` (x could be anything).
ardumontUnsubmitted
Done
Inline Actions

file_name: filename whose extension needs to be checked.

ardumont: file_name: filename whose extension needs to be checked.
ardumontUnsubmitted
Done
Inline Actions

exten`s`ion

ardumont: exten`s`ion
About · Developer information · Legal