[internal] Add a release artifact size verification step. #13234
Conversation
[ci skip-rust] [ci skip-build-wheels]
|
Currently reports: |
| if versions_by_freshness_ascending[-1].size_mb > available_mb: | ||
| break | ||
| available_mb -= versions_by_freshness_ascending.pop().size_mb | ||
| return versions_by_freshness_ascending |
There was a problem hiding this comment.
I'll probably add a safeguard here to confirm that it never suggests deleting more than N versions. But would be interested in any other safeguards folks can think of.
There was a problem hiding this comment.
I don't see any safeguard against recommending deleting non-dev releases? I would imagine we want to think 1000 times before deleting recent dev releases, rcs or, of course, stable releases.
There was a problem hiding this comment.
When we get into very recent dev releases (it's currently recommending deleting 5+ month old dev releases) things definitely get hairy, yea. Currently it prioritizes removing dev releases before rcs before stable releases.
But perhaps we need a recency safeguard, where we rule out deleting anything younger than X months old? The trouble with that right now is that we will quickly move on to deleting stable releases... admittedly, there are some very old stable releases (pre-1.0.0 basically), which we could manually delete to free up space in the more recent releases to give us more headroom.
There was a problem hiding this comment.
You could add constraints per release kind to that, so instead of time based, you could say
(blacklist vs whitelist simply reverse the constraint...)
stable: <1.30
rc: <2.0
dev: <2.2
As time is less important than release cadence, I think...
There was a problem hiding this comment.
Agreed that doing something PackageVersionType-dependent would make more sense, but it would also be more complicated. I'm going to keep this simpler for now, and have added a minimum age to be eligible.
|
Hopefully we can render this moot some day soon by deploying Pants as a binary, instead of as a dist. We'll still need to deploy the plugin API as a dist, but that could be a lot smaller since it wouldn't include the native engine. |
| stale_version for package in PACKAGES for stale_version in package.stale_versions() | ||
| ] | ||
| if stale_versions: | ||
| print("\n".join(f"Stale:\n {sv}" for sv in stale_versions)) |
There was a problem hiding this comment.
This would be more useful if you implemented __str__ for the type. The comment you posted was a bit chatty.
After encountering another issue with the #12397 covers this, but: switching to shipping a binary resolves it. If we ship a PEX (which we already deploy to Github as part of our release process), the PEX is stable with embedded In the past there were some objections to shipping a PEX due to performance, but with venv mode, that should no longer be a concern. |
|
Great idea to exclusively ship a PEX. How would we handle plugins - release a library-only dist without the native code? Also don't include the console script / entry point. |
|
I'll step aside from this review. I'd still prefer deleting for past and moving forward as described in the OP in #11614. I think we're doing a bunch of dancing here for 0 users benefit in reality. |
At this point we have deleted all of the dev releases aside from the ones mentioned in #13234 (comment) (and newer): and that has not been sufficient. The thing that we realized the other day that means we have relatively little headroom is that the size limit is per package... i.e. we're hitting the cap for |
Hm, very good question. Because testing a plugin does actually need the native code. |
|
Ok. Option #37: Since no-one installs pantsbuild.pants via pip or otherwise except our own setup script / pants PEX creation process, there is 0 reason to use the PyPI index for publishing - we could use our own and foot the bill. |
Yea, that's true aside from the point @Eric-Arellano raised: 3rdparty plugin code does actually need to resolve Pants (using Pants) in order to develop against it. But, the So... perhaps the existing support for resolving from S3 in the |
… can safely be deleted. # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
|
Until we swap to distributing a PEX (which definitely seems like the way forward in the next few weeks), I think that we should land this. Have added a minimum age safeguard. And: this is still a manual step, so we'll lean on releasers to actually apply it and discuss things before deleting them if they have concerns. |
|
What does "in case someone is developing a plugin" mean? If I'm developing a plugin and I use, say vscode, how am I expected to setup my project and get symbols, etc? Are you expecting us to publish instructions on how to build the venv pointing to our S3 find-links repo? If so that precludes Poetry users IIUC. They'll need to break their typical workflow and use Pip to build the vscode venv. |
The As to creating a virtualenv for plugin development: that's effectively the same as any other IDE usage with Pants: to get a properly scoped venv in the context of multiple different resolves and requirements, you need to use something like the |
|
Aha, so your implicit assumption is to develop a Pants plugin you must use Pants. I was not assuming that. Seems reasonable enough though. |
Before beginning to automate deleting our "most stale" artifacts, add a manual verification step that encodes which artifacts we prefer to delete (
devreleases beforercsbefore stable, then sorted by last upload date), and suggests deleting them before attempting to run a release.This will likely be annoying, but verifying that the results look sane in the short term will be a blocker for automating deletion. The next step in release automation will probably be getting the pantsbuild bot's PyPI credentials into Github Actions secrets and starting to 1. run releases there, 2. delete stale releases there (if we think we can be comfortable with that).
[ci skip-rust]