Fixed over publishing, improved publication tests, and increased publication speed - #363
Conversation
74218c8 to
bd33d49
Compare
gerrod3
left a comment
gerrod3
left a comment
There was a problem hiding this comment.
This publication optimization is pretty much a full refactor and it's quick. I did a test with a repository with 2000 packages and 375,000 releases and it took 29 mins for the current way, and 70 secs for this new optimized way. With N as the number of packages and M as the number of releases, we currently do 1 + N + M database saves and 1 + N + 2M database queries (1 + N + M queries if sync is immediate) with all of the M queries just getting one element. With the new optimization we will do 2 + N database saves and 3 database queries (for both on_demand and immediate sync, can be dropped to 2, see comments below). The memory usage could possibly grow quite large, but that can be optimize if we break up the large database saves into smaller batches.
| release_content_artifacts = python_models.PythonPackageContent.objects.filter(pk__in=publication.repository_version.content).values_list("contentartifact", flat=True) | ||
| remote_artifacts = models.RemoteArtifact.objects.filter(content_artifact__in=release_content_artifacts).values_list("content_artifact", "sha256").iterator() | ||
| checksums = {ca: sha for ca, sha in remote_artifacts} # This can grow to 4 million elements if fully PyPI synced |
There was a problem hiding this comment.
The remote_artifacts query can be removed (along with release_content_artifacts although that query shouldn't be evaluated) if we add sha256 to the PythonPackageContent model. We have to get the remote artifacts incase the sync is on_demand because the download links require the sha256 be present. While the query has been optimize to be quick and take up little memory the dictionary checksums that I create from it is really large and can grow to be up to 4 million elements (number of releases available on PyPI). I need to do more tests to check how bad the memory usuage is, but I have a feeling this optimization is needed to prevent going OOM.
There was a problem hiding this comment.
Potentially we should do that anyways if filename is not a workable (global) uniqueness constraint.
| ind = 0 | ||
| current_name = index_names[ind][0] | ||
| package_releases = [] | ||
| published_artifacts = [] # This list can grow up to 4 million elements for full PyPI, can be cleared if we do multiple bulk_creates |
There was a problem hiding this comment.
This list can also grow up to 4 million elements, but it can be cleared if I bulk_create after every N releases.
| content_artifact_id=content_artifact | ||
| )) | ||
| write_project_page() # Write the final project's page | ||
| models.PublishedArtifact.objects.bulk_create(published_artifacts) # maybe play around with batch size? |
There was a problem hiding this comment.
Not so sure if this bulk_create will do well with a list of 4 million elements, but it worked fine for my test with 375,000. It's possible there is some hard-coded batch-size limit under the hood that will choose the best number for us, but I'm not sure.
There was a problem hiding this comment.
Django does do internal batching and enforces a max batch size, but I would set one manually here. 2000 would be fine.
https://github.com/django/django/blob/main/django/db/models/query.py#L547-L548
Although - and I might be misunderstanding pass_through, I thought that the purpose was to avoid manually saving published artifacts?
There was a problem hiding this comment.
This is from before I was setting pass_through. I got rid of this and the extra published_artifacts as they are no longer needed.
gerrod3
changed the title
2135352 to
10548da
Compare
[noissue]
00b8dba to
44a3a1e
Compare
44a3a1e to
56b0054
Compare
[noissue]
56b0054 to
4cad081
Compare
2 participants
fixes: #347
fixes: #362