feat(list-versions): show cooldown status and upload timestamps#1127
feat(list-versions): show cooldown status and upload timestamps#1127shifa-khan wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements the cooldown details phase of the release-age cooldown feature ( Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fromager/commands/package.py (1)
196-245: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd per-requirement logging context in
list_versions
list_versionslogs requirement-specific messages but does not uselog.req_ctxvar_context(req), so contextual logging is inconsistent with other command paths.Suggested fix
- logger.info(f"Looking up versions for {req.name}") - if req.specifier: - logger.info(f"Filtering versions with specifier: {req.specifier}") + with log.req_ctxvar_context(req): + logger.info(f"Looking up versions for {req.name}") + if req.specifier: + logger.info(f"Filtering versions with specifier: {req.specifier}") - pbi = wkctx.package_build_info(req) - override_sdist_server_url = pbi.resolver_sdist_server_url(sdist_server_url) + pbi = wkctx.package_build_info(req) + override_sdist_server_url = pbi.resolver_sdist_server_url(sdist_server_url) - include_sdists, include_wheels = parse_distribution_option( - distribution_type, - pbi, - ) + include_sdists, include_wheels = parse_distribution_option( + distribution_type, + pbi, + ) - provider = overrides.find_and_invoke( + provider = overrides.find_and_invoke( ... - ) + ) + ...As per coding guidelines:
src/fromager/**/*.py: “Usereq_ctxvar_context()for per-requirement logging”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fromager/commands/package.py` around lines 196 - 245, The requirement-specific logs in this block should be executed inside the per-requirement logging context; wrap the code that starts after Requirement(requirement_spec) (the logger.info calls, pbi = wkctx.package_build_info(req), parse_distribution_option, overrides.find_and_invoke(...), provider.find_matches(...) and the candidates/versions handling) in the context manager returned by log.req_ctxvar_context(req) so that all requirement-scoped log messages use the req context; locate the code around Requirement(...) and wrap the subsequent logic (up through computing versions) with with log.req_ctxvar_context(req): to ensure consistent contextual logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fromager/commands/package.py`:
- Around line 263-270: The table branch ignores the requested output
destination: in the match on output_format you call
_export_versions_table(version_rows, req.name, cooldown) which always prints to
stdout, so update the call and the function to accept an output file/stream
(e.g. add an output parameter) and forward the provided output handle used by
the json/csv branches; modify _export_versions_table signature and all call
sites (including the other occurrence around lines 402-422) to accept and write
to the passed output instead of writing directly to stdout.
---
Outside diff comments:
In `@src/fromager/commands/package.py`:
- Around line 196-245: The requirement-specific logs in this block should be
executed inside the per-requirement logging context; wrap the code that starts
after Requirement(requirement_spec) (the logger.info calls, pbi =
wkctx.package_build_info(req), parse_distribution_option,
overrides.find_and_invoke(...), provider.find_matches(...) and the
candidates/versions handling) in the context manager returned by
log.req_ctxvar_context(req) so that all requirement-scoped log messages use the
req context; locate the code around Requirement(...) and wrap the subsequent
logic (up through computing versions) with with log.req_ctxvar_context(req): to
ensure consistent contextual logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 028ca9d6-ba8a-480e-8b7a-b9666d871f34
📒 Files selected for processing (2)
src/fromager/commands/package.pytests/test_list_versions.py
|
Tested the changes locally: $ fromager --min-release-age 20 package list-versions --details --format json click
13:52:27 INFO Looking up versions for click
13:52:27 INFO Found 57 version(s)
},
{
"package": "click",
"version": "8.3.0",
"upload_time": "2025-09-18T17:32:23.696258+00:00",
"age_days": "231",
"cooldown": "allowed"
},
{
"package": "click",
"version": "8.3.1",
"upload_time": "2025-11-15T20:45:42.706404+00:00",
"age_days": "172",
"cooldown": "allowed"
},
{
"package": "click",
"version": "8.3.2",
"upload_time": "2026-04-03T19:14:45.118083+00:00",
"age_days": "33",
"cooldown": "allowed"
},
{
"package": "click",
"version": "8.3.3",
"upload_time": "2026-04-22T15:11:27.506141+00:00",
"age_days": "15",
"cooldown": "blocked"
}
]
With a 20-day cooldown, click 8.3.3 (15 days old) is correctly marked as blocked while 8.3.2 (33 days old) is allowed. |
shifa-khan
force-pushed
the
list-versions-1078
branch
from
88f414f to
a967ee6
Compare
shifa-khan
force-pushed
the
list-versions-1078
branch
5 times, most recently
from
ba62e44 to
22685c7
Compare
shifa-khan
force-pushed
the
list-versions-1078
branch
2 times, most recently
from
39e1053 to
650cd2b
Compare
|
We need to add some e2e tests, it looks like. The version-only listing works properly, but the table formatter does not filter by age. |
|
| ) -> None: | ||
| """Export version details as CSV.""" | ||
| if output: | ||
| with open(output, "w", newline="") as outfile: |
There was a problem hiding this comment.
This duplicates the full DictWriter block across if - else branches.
If we determine the output stream earlier we can avoid the duplication.
outfile = open(output, "w") if output else sys.stdout
try:
writer = csv.DictWriter(.....)
writer.writeheader()
writer.writerows(data)
finally:
if output:
outfile.close()
There was a problem hiding this comment.
Or simpler: use click.File in the argument and pass io.TextIO type around.
There was a problem hiding this comment.
@smoparth @tiran The initial version matches the pattern in list_overrides._export_csv to stay consistent. If both commands need further cleanup (e.g., click.File or try/finally), I can address that in a follow-up PR. Note that click.File has drawbacks, such as not supporting the newline="" requirement for CSV files.
@dhellmann Thanks for pointing that out. Question on scope - the issue says "list-versions should display upload timestamps and indicate which candidates are blocked by the cooldown policy." Should the detail formats (table/csv/json) still show blocked versions with a "blocked" label so the command can serve as a diagnostic tool? With a short cooldown like 3-7 days there could be versions available on PyPI but blocked by the policy, and it'd be useful to see those rather than have them silently filtered. |
Ah, that's a great point. I was focused on making it work the same as the default view, but you're right, it shows that the package would not be included so that's actually a better UX. Ignore my comment. |
@tiran How about "blocked" for blocked entries and "available" for allowed? Leaving the column blank could confuse users. Yanked versions are already filtered out by the resolver. I verified this with the click package — version 8.2.2 is yanked on PyPI and doesn't appear in the output. |
|
@dhellmann |
Yes, I think the plain version list and the requirements.txt formatter should filter and the others that provide more details should not filter.
There's an option to look for both types: https://github.com/python-wheel-build/fromager/blob/main/src/fromager/commands/package.py#L37 And the command uses the plugin to get a resolver, if there is a custom one: https://github.com/python-wheel-build/fromager/blob/main/src/fromager/commands/package.py#L146 So I think it's possible to do the full range of querying we expect? |
Right, that's what I figured. list-versions already has --distribution-type and goes through get_resolver_provider, so the full querying range is covered. I ran into this with torch (which only publishes wheels) and --distribution-type wheel handled it as expected. |
shifa-khan
force-pushed
the
list-versions-1078
branch
2 times, most recently
from
3c8cd81 to
bf88758
Compare
package list-versions now shows upload_time, age in days, and cooldown status for each version. --format controls the output: versions (default) and requirements print a filtered list excluding blocked entries; table, csv, and json include the full detail columns. This replaces the previous --format-as-requirements flag. --ignore-per-package-overrides shows what the global --min-release-age would block without per-package exemptions. Co-authored-by: Cursor <cursoragent@cursor.com>
shifa-khan
force-pushed
the
list-versions-1078
branch
from
bf88758 to
cb41274
Compare
4 participants
package list-versions now shows upload_time, age in days, and cooldown status for each version.
--format controls the output: versions (default) and requirements print a filtered list excluding blocked entries; table, csv, and json include the full detail columns. This replaces the previous --format-as-requirements flag.
--ignore-per-package-overrides shows what the global --min-release-age would block without per-package exemptions.
Closes: #1078