Bump click upper bound to <8.4#7324
Open
GeigerJ2 wants to merge 13 commits into
Open
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7324 +/- ##
==========================================
- Coverage 80.26% 80.26% -0.00%
==========================================
Files 577 577
Lines 45497 45522 +25
==========================================
+ Hits 36514 36532 +18
- Misses 8983 8990 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Collaborator
|
the CI denied your proposal |
Collaborator
Author
|
Collaborator
Author
Now it says yes! Just gotta actually look at the vibe-coded code myself 😅 
|
As of click 8.3, `Option.consume_value` re-prompts for options whose value is sourced from `default_map` when the option has a prompt and is required. In non-interactive mode, the prompt call falls through to `get_default`, which for `InteractiveOption` returns the `contextual_default` — silently overwriting the value provided via a config file. This broke `verdi setup --non-interactive --config profile.yaml`, as the `db_username`, `db_name`, and other config-sourced values were replaced by the auto-computed defaults (e.g. `aiida_qs_<user>_<hash>`), causing connection failures against postgres roles that do not exist. Override `InteractiveOption.prompt_for_value` so that in non-interactive mode it returns the `default_map` value if one is present, falling back to `get_default` otherwise. This restores the pre-8.3 behaviour where config-file values take precedence over the contextual default.
`click >= 8.3` pre-populates `ctx.params` with an `UNSET` sentinel for
parameters that have not yet been processed. The sentinel is truthy,
so the pattern
user = ctx.params.get('user', None) or orm.User.collection.get_default()
no longer falls back to the default user: it yields `UNSET`, which is
later dereferenced as `user.pk` and raises
AttributeError: 'Sentinel' object has no attribute 'pk'
This breaks `verdi computer configure core.ssh` when the option order
causes the `user`, `computer`, or `non_interactive` parameters to be
looked up before they have been processed (e.g. from a contextual
default of another option), which surfaces during the `test-install`
CI job.
Normalise the relevant context params via an explicit `isinstance`
check, returning `None` for any value that is not of the expected
type (including the `UNSET` sentinel).
The previous fix only returned config-file values from `prompt_for_value` when running in non-interactive mode (`-n`/`--non-interactive`). But `verdi computer configure` in CI runs without `-n`, relying on `--config` alone. Click 8.3 re-triggers `prompt_for_value` for options whose value was sourced from `default_map`. In interactive mode this fell through to the actual prompt, which aborted in CI (no terminal on stdin). Move the `default_map` check before the `is_interactive` gate so config-file values always short-circuit prompting, restoring pre-click-8.3 behavior.
Click 8.3 changed `value_is_missing` to only flag the `UNSET` sentinel, not `None`. This broke the `!` (ignore default) flow for required options: `process_value` set the value to `None` and relied on `super().process_value` to raise `MissingParameter`, but the missing-value check no longer fires for `None`. Handle required options explicitly before delegating to `super()`: if the option is required and we are in interactive mode, print the error and re-prompt immediately.
GeigerJ2
force-pushed
the
deps/click-8.3
branch
from
4bd98f0 to
c28aaf1
Compare
Use `is_required(ctx)` instead of bare `self.required` so `required_fn` callbacks are respected when re-prompting for `!` on required options. Extract `_get_processed_param` as a standalone module-level helper in `transports/cli.py` instead of a nested closure. Add `default_map` guard to `TemplateInteractiveOption` to match the fix already applied to `InteractiveOption`. Add tests for `required_fn` re-prompt, `default_map` precedence (interactive and non-interactive), and `default_map` overriding `contextual_default`.
Click 8.3 sets `get_parameter_source` to `DEFAULT` even when the actual value in `ctx.params` is the `UNSET` sentinel (e.g. `USER` option whose `default=UNSET`). Checking source alone was not sufficient: `user` passed the source check but held `UNSET`, a truthy non-User object that skipped the `or` fallback and crashed on `.pk` access. Add an explicit `value is UNSET` guard after the source check. Import the sentinel with a try/except fallback for click < 8.3 compatibility.
Extract `resolve_param` from `transports/cli.py` into `cmdline.utils.common` and apply it to every `ctx.params` read that runs during parameter processing (contextual defaults, prompt/required callbacks, validators, type converters). In click 8.3, unprocessed parameters are pre-populated with a truthy UNSET sentinel. Raw `ctx.params` reads in callbacks could see UNSET instead of None, silently passing boolean/None checks or crashing on attribute access. Guarded call sites: - `InteractiveOption.is_interactive` (non_interactive) - `interactive_default` (non_interactive, user, computer) - `is_on_computer` (on_computer) - `validate_label_uniqueness` (computer, on_computer) - `get_job_resource_cls` (scheduler) - `MpirunCommandParamType.convert` (scheduler) Also add a test for `TemplateInteractiveOption` with `default_map`.
`test_validate_label_uniqueness` constructs a `click.Context` manually and sets `ctx.params` directly. After the switch to `resolve_param`, the helper checks `get_parameter_source` before reading `ctx.params`, so the test must also register sources for the parameters it provides.
danielhollas
approved these changes
Apr 28, 2026
Collaborator
danielhollas
left a comment
danielhollas
left a comment
There was a problem hiding this comment.
Ooof, the changes here are quite subtle. 😨
I can't say I went deep enough to fully understand everything (and I haven't looked much at the added tests), but as long as some manual testing is done to verify that things work (especially around the interactive use which seems to impacted the most?), this looks fine. Thanks!
|
|
||
| ctx = click.Context(cmd_code.setup_code) | ||
| ctx.params = {'on_computer': False} | ||
| ctx.set_parameter_source('on_computer', click.core.ParameterSource.COMMANDLINE) |
Collaborator
There was a problem hiding this comment.
Why is this required?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While exploring some of the stack proposed by potential contributors to GSoC, I noticed that some of the AI libraries require click 8.3. Hence, I thought why not try and loosen our pin.
At least local CLI tests seem to run through. Just opening the PR for the full CI pipeline.
UPDATED PR DESCRIPTION
Bump click upper bound from
<8.3to<8.4, fixing three breaking changes introduced by click 8.3's newUNSETsentinel (an enum member that distinguishes "no value provided" from "explicitlyNone"). All fixes are backwards-compatible with click 8.2 (identical runtime behavior).1. Config-file values re-prompted instead of accepted
Click 8.3 added
ParameterSource.DEFAULT_MAPto the prompt trigger condition inOption.consume_value. Values provided via--config file.yaml(which populatectx.default_map) were silently accepted in 8.2 but now triggerprompt_for_value, aborting in non-interactive environments like CI.InteractiveOption.prompt_for_valueandTemplateInteractiveOption.prompt_for_valuenow checkctx.default_mapfirst and return the config-file value immediately, before any interactive/non-interactive branching. On click 8.2 this code path is never reached (click doesn't callprompt_for_valueforDEFAULT_MAPsources), so behavior is unchanged.Files:
src/aiida/cmdline/params/options/interactive.py2.
ctx.paramspre-populated with truthyUNSETsentinelsClick 8.3 changed
Parameter.consume_valuefromopts.get(self.name)toopts.get(self.name, UNSET), and pre-populatesctx.paramswithUNSETfor unprocessed parameters. The sentinel is truthy (bool(UNSET)isTrue), which brokeinteractive_defaultin the transport CLI: it readsnon_interactive,user, andcomputerfromctx.paramsto compute defaults, andUNSETpassed boolean/None checks it shouldn't have.New module-level helper
_get_processed_param(ctx, name, default)checksctx.get_parameter_source(name)before readingctx.params. Returns the safe default if the parameter hasn't been processed yet. On click 8.2,get_parameter_sourcealready returnsNonefor unprocessed params and the actual source for processed ones, so the helper produces the same values as the old directctx.paramsaccess.Files:
src/aiida/transports/cli.py3.
!on required options silently accepted asNoneClick 8.3 changed
Parameter.value_is_missingfromvalue is Nonetovalue is UNSET.InteractiveOptiontranslates the!prompt input toNoneand relied onsuper().process_valueraisingMissingParameterfor required options, triggering a re-prompt. With 8.3,Nonepasses the missing-value check and is silently accepted.The fix checks requiredness explicitly before delegating to
super(), usingself.is_required(ctx)(fromConditionalOption) instead of the bareself.requiredflag. This also fixes a pre-existing bug:ConditionalOptionsetsself.required = Falsewhenrequired_fnis provided, so the old raise-and-catch path viasuper().process_valuenever re-prompted forrequired_fn-required options, even on click 8.2. The new explicit check handles bothrequired=Trueandrequired_fncorrectly.Files:
src/aiida/cmdline/params/options/interactive.pyOther files
tests/cmdline/params/options/test_interactive.py: three new tests coveringrequired_fnre-prompt on!,default_mapprecedence (interactive + non-interactive), anddefault_mapoverridingcontextual_default