Conversation
|
Will update to jump to the v3 key containing per-format subsections. |
| continue | ||
|
|
||
| return new_repodata_packages, new_repodata_conda_packages | ||
| if v3: |
There was a problem hiding this comment.
this means if v3 is true, older sections are ignored, and new_packages will be empty:
new_packages = {
"packages": {},
"packages.conda": {},
"packages.whl": {},
}
Is that intended?
There was a problem hiding this comment.
If v3 is True, then we put all packages into the new_v3_packages dict and "packages", "packages.conda" will be empty. This is simpler than the "migrated-since" in the draft CEP.
I think the "new_" prefix is confusing here since it's just "all of them"; this name might be left over from old-conda-index which didn't pull all packages from cache every time.
There was a problem hiding this comment.
That doesn't follow the spirit of the CEP, though. The point is to be backwards compatible, so this will effective leave incompatible clients behind.
There was a problem hiding this comment.
This was written to follow experimental wheel work and to defer the spirit of the CEP.
| if section_and_key is None: | ||
| log.warning("%s has unsupported package extension", path) | ||
| continue | ||
| key, v3_path = section_and_key |
There was a problem hiding this comment.
the name key is used to refer to the second element of the returned tuple here. It's a bit confusing.
We should change it to: section, key = section_and_key.
| if section_and_key is None: | ||
| log.warning("%s has unsupported package extension", path) | ||
| continue | ||
| key, v3_path = section_and_key |
|
The main objective of this PR is to add a top level key called "v3" to I have added a few comments but the PR looks good to me. |
| } | ||
| } | ||
| if v3 | ||
| else {"packages": {}, "packages.conda": {}} |
There was a problem hiding this comment.
shard should always include the packages, packages.conda keys
| } | ||
| } | ||
| if v3 | ||
| else {"packages": {}, "packages.conda": {}} |
There was a problem hiding this comment.
It would be nice if the boring "constructing the big dict" could be moved to the abstract base class instead of duplicated per database.
There was a problem hiding this comment.
yes, agreed. There is a lot of duplicate code. We can either write a function to create these dictionaries in this PR or do that in a subsequent PR.
I am okay with addressing this in the next PR.
| show_default=True, | ||
| ) | ||
| @click.option( | ||
| "--repodata-v3/--no-repodata-v3", |
There was a problem hiding this comment.
Eventually, there'll be a v4, v5... should we instead have a non-boolean flag like --max-repodata-version or something?
There was a problem hiding this comment.
Are repodata versions something that are sequential, or are they more like CPU flags where we may use any set of them?
There was a problem hiding this comment.
Hey @dholth, they are sequential and we would probably keep all previous versions as well for backwards compatibility (although maybe there is a deprecation time eventually?)
|
There's no repodata v3 yet, so I'm not sure what this is trying to do (I guess adding support for the wheels preview). In that case, maybe it should leave conda packages alone for now and simply populate |
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
Yes this is only intended to support the wheels preview. |
ryanskeith
left a comment
There was a problem hiding this comment.
Overall, I am not seeing any huge red flags about going forward this way. I don't think anyone is a fan of having version 2's of code but it is a necessary evil at times to get things working well.
| for name, shard in cache.indexed_shards(): | ||
| shard_data = compressor.compress(sqlitecache.packb_typed(shard)) | ||
| shard_hash = hashlib.sha256(shard_data).digest() | ||
| v3_data = { |
There was a problem hiding this comment.
Is the concern processing time? If so, could this be an elective post processing function?
| for name, shard in cache.indexed_shards(): | ||
| shard_data = compressor.compress(sqlitecache.packb_typed(shard)) | ||
| shard_hash = hashlib.sha256(shard_data).digest() | ||
| v3_data = { |
There was a problem hiding this comment.
Ah, I see. It is part of the spec. I am not sure what the intent of this spec provides.
Mention conda-pypi
| if fn in repodata[key]: | ||
| repodata[key][fn]["revoked"] = True | ||
| repodata[key][fn]["depends"].append("package_has_been_revoked") | ||
| records = repodata.get(key, {}) |
There was a problem hiding this comment.
repodata patch doesn't work with wheel or v3
Co-authored-by: Dan Yeaw <dan@yeaw.me>
| package_sections = { | ||
| ".tar.bz2": "packages", | ||
| ".conda": "packages.conda", | ||
| ".whl": "packages.whl", |
There was a problem hiding this comment.
A little obsolete since there isn't a "packages.whl" in current specs but we also use it to populate IndexedPackages / IndexedShard dataclasses.
Co-authored-by: Dan Yeaw <dan@yeaw.me>
Co-authored-by: Dan Yeaw <dan@yeaw.me>
* cep-21 run exports in shards * update typing
Description
Ability to output wheel metadata to monolithic and sharded repodata.
Does not create the wheel metadata; a different program must have already inserted it into our database before repodata generation runs.
Per the pattern in https://github.com/conda-incubator/conda-pypi-test or the draft repodata v3 at conda/ceps#146
See also #262
Fix #268
AI-assisted
Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?