the argument is not a file, it's a parsed structure

It's not clear what this does. Can you add an (abridged) example directory listing of what you expect in dir_path in the docstring ?

Please add documentation on what the arguments do.

The status code alone may not be sufficient to debug the problem. Please also provide the body of the response in exceptional cases to make them easier to diagnose.

Why have the client pass the project_url *and* the project name? Couldn't the URL be constructed like in the release function?

This base URL should be registered somewhere else. I'd suggest either a class attribute or an instance attribute set by a parameter (there's more than one Python package repository in the wild).

As this does more than fetching, I'd rename this function prepare_release_artifact, and possibly break it down in three distinct "fetch" (also handles caching and integrity control), "uncompress" and "metadata gathering" steps?

To be more generally useful, I'd let the caller control where the archive is fetched and uncompressed, rather than needing to build a known directory name between the caller and this, and risking both to get out of sync.

this whole method could benefit from using the stream=True argument to requests.get : http://docs.python-requests.org/en/master/user/advanced/#body-content-workflow

• doesn't put the whole tarball in memory
• allows you to check the size before fetching the first byte

This would still use r.iter_content(), you just need to use r.headers['content-length'] (?) instead of len(r.content)

You should compute all hashes when processing the file initially (unfortunately I don't think our hashutil provides this functionality natively), this would save one full file read, and probably the need for convert_to_hex as well.

release.update(artifact)? or more sensibly release['artifact'] = artifact?

Is the client argument ever used? I don't seem to see it used in this diff.

This may fail if the tarball contained files with bogus permissions (e.g. directories without write access). We may gain from a permission normalization phase in the decompression stage.

I wonder why all objects have an underscore in this function?

Creating instance attributes in a method is considered pretty bad form; Please move those initializations to the __init__ method, or at least to the prepare method.

Those should also probably be id -> object dicts to provide some deduplication and memory savings in the following loop (for contents, the id should be a compound key with all hashes).

This loop completely bypasses the incremental loading capabilities implemented in the base loader. This risks the loader using too many resources as it will materialize all objects in memory before sending them to the database, most notably on initial imports.

The incremental pattern is currently used in the Debian loader.

The idea is that the prepare method talks to the database and PyPI, and gathers what work needs to be done in some instance attributes. fetch_data and store_data get called in a loop, with the provision that fetch_data only fetches packages one at a time, and store_data only pushes the data for that package in a queue (which gets automatically flushed when it's too full, therefore keeping memory usage limited).

The revision generation should probably be moved to a method of the PyPIProject class, or to a function in the same module.

Where does the data come from, and what schema does the project conform to?

Where does this data come from? Please be explicit that the return value is in the shape the Software Heritage archive expects.

permit -> allow

Isn't PyPIClient doing that?

It'd be useful to have some expected usage patterns for this class.

Please also add documentation for the arguments for class instantiation.

client + project and project_metadata_url are possibly redundant

Possibly re-inline the info function, as it's not being tested separately.

This really filters the files within a release?

again with the stuck _ key ;)

From what I can tell, this uncompresses the multiple files for a single release, is that correct?

If that's so, then it should be named accordingly.

and

Is it a name or a version? I see it gets packed into a 'name' entry in the flattened dict, so an example would be useful.

Is this really useful? AIUI the releases list (which should probably be a release_files list) comes from the PyPI API directly, so it'd make sense for the PyPIClient class to understand it directly, wouldn't it?

If it is really useful, then the functionality should likely be moved to a separate function/staticmethod to ease testing.

flattened

Are there other digests that can be checked? If so we should avoid losing them, and check them on the client side.

known_files

Task, not Tsk

yes.

yes

Probably, it was only for debug purposes so i did not push further.

Unfortunately, it's for the tests.

It's already done in the swh.core.tarball.uncompress call [1]

[1] https://forge.softwareheritage.org/source/swh-core/browse/master/swh/core/tarball.py$165-173

To explicit it's private to that method, i agree it's not needed.
I'll keep only those for the methods then.

(it's not really only that function, e.g. _known_releases method also).

Creating instance attributes in a method is considered pretty bad form; Please move those initializations to the init method, or at least to the prepare method.

I don't like it either.
But i like keeping the state local to where it's needed (here fetch_data, store_data).
In the prepare method (or init), it's not so local anymore...

I'm also doing this because the way the loader core works...
I'd prefer to pass along those that state to the store_data method...
But i don't feel like changing that now.

Yes, i struggled on this.
First implementation broke the fetch_data, store_data pattern as fetch_call did not use any state and directly called the maybe_load_... methods. So store_data was unused.

The incremental pattern is currently used in the Debian loader.

Will take a closer look there. Thanks.

rahhhhh

Isn't PyPIClient doing that?

Yes, it's pypiclient (as collaborator as this class) which does it.

It'd be useful to have some expected usage patterns for this class.
Please also add documentation for the arguments for class instantiation.

Yes, will do.

well, only md5 ;)

Well right now, i won't split it yet.
But i will do the other improvments proposed.

That sounds good, thanks.

yeah, ok.

(unfortunately I don't think our hashutil provides this functionality natively)

I think hashutil.hash_file allow that.

(Not for the convert_to_hex part though, hard-coded to compute the binary format).

release.update(artifact)?

Huh, right...

or more sensibly release['artifact'] = artifact?

No, the release here is actually the artifact... (that's one of your point earlier ;)
I'm renaming the release name here.

Yes, but later.

Well, the name is the version.

ymmv, we have some with Task, Tsk (that'd be my fault if we were pointing finger) and nothing...

Maybe nothing is good enough (e.g. task_type with the backend as the fqdn, we already see the .tasks. module).

And done \m/

I have opened D410 to open an hash_stream and add an option to require hexdigest (in swh.model.hashutil).
And i will update this diff with its usage here.

If you have a minute, it'd be nice.

Thanks for your time.