More model fixes stemming from select model changes in 23351. - #23368
Draft
jmchilton wants to merge 1 commit into
Draft
More model fixes stemming from select model changes in 23351.#23368jmchilton wants to merge 1 commit into
jmchilton wants to merge 1 commit into
Conversation
basic.py passes a type-specific default to parse_optional; factory.py called it bare. Two divergences followed. genomebuild - GenomeBuildParameter subclasses SelectToolParameter, so runtime reads parse_optional(self.multiple). The model read parse_optional(), so `genomebuild multiple="true"` modelled as required. Same shape as the select fix in galaxyproject#23351, five lines away in the same function. drill_down - the branch never called parse_optional at all, and _common_param_kwargs only carries label/help, so an explicit optional="true" was dropped and the model fell back to the base default. DrillDownParameterModel also never consulted optional, so honour it in py_type and request_requires_value. Note multiple does NOT imply optional here: DrillDownSelectToolParameter calls ToolParameter.__init__, not SelectToolParameter.__init__. Nothing in the tree sets drill_down optional="true", so this cannot regress an existing tool; gx_drill_down_exact_optional.xml is added to exercise it. Also closes coverage gaps left by galaxyproject#23351. The states that actually flipped there were test_case_xml/test_case_json null, which had no cases; request::null flips for gx_select_multiple_no_options_validation but not for its single-select sibling, so that surface is worth asserting. The new tool had no execution coverage - it now rides the existing multi-select test alongside gx_select_optional_no_options_validation. Restores the workflow_step question that was deleted rather than answered: py_type_workflow_step is unconditionally optional, which is a separate issue from optional-by-default. Fixes ["hg18", hg19"] in the genomebuild spec - the missing quote made YAML parse the value as the string 'hg19"'. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G95icoGkoDKbwPotGnvQpk
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
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.
PR message from agent. -John
Follow-up to #23351, which made
select multiple="true"optional by default in the parametermodel. That fix was right —
galaxy.xsdalready documented the rule ("Defaults tofalseexcept when the
typeattribute value isselectandmultipleistrue") andSelectToolParameterhas readparse_optional(self.multiple)since 2013. But the model layerderives
optionalin eleven independent places, and reviewing that PR turned up two moresites that had drifted from runtime the same way.
The divergences
genomebuild—GenomeBuildParametersubclassesSelectToolParameter, so at runtime itinherits
parse_optional(self.multiple). The model calledparse_optional()bare and readmultipleon the very next line.genomebuild multiple="true"therefore modelled as requiredwhile the runtime treated it as optional. This is the same bug #23351 fixed, five lines away in
the same function.
drill_down— the branch never calledparse_optionalat all, and_common_param_kwargsonly carries
label/help, so an explicitoptional="true"was silently dropped and the modelfell back to the base default of
False.DrillDownParameterModelalso never consultedoptional, so this wires it intopy_typeandrequest_requires_value.Note the asymmetry: for
drill_down,multipledoes not imply optional.DrillDownSelectToolParameter.__init__callsToolParameter.__init__rather thanSelectToolParameter.__init__, so it reads a bareparse_optional()with no multiple-deriveddefault. The fix matches that rather than copying the select rule.
No tool in the tree sets
drill_down optional="true", so that half cannot regress an existingtool —
gx_drill_down_exact_optional.xmlis added to exercise it.Coverage gaps from #23351
Measuring what that one-line change actually altered — 9 state representations × 4 value shapes,
run with the commit applied and reverted — showed the affected states were narrower than the
title suggests.
request,request_internal,landing_request,job_internal,job_runtime,workflow_stepandworkflow_step_linkedare byte-identical for a plain multiple select,because those paths already read
self.optional or self.multipleandrequest_requires_valuereturns
Falseunconditionally. What actually moved:test_case_xml/test_case_jsonnull, for every multiple select — the widest-reachingchange, and it had no spec cases. Added.
request::nullfor theno_options-bearing tool, which is where the real bug was. It flipsthere but not for the single-select sibling, so that surface is worth asserting. Added.
gx_select_multiple_no_options_validationhad no execution coverage at all. It now rides theexisting multi-select test, following how
gx_select_optional_no_options_validationis stackedonto the single-select test 15 lines above.
Also restores a comment #23351 deleted rather than answered — the
workflow_step_validquestionis about
py_type_workflow_stepbeing unconditionally optional, which is a separate issue fromoptional-by-default.
Drive-by
gx_genomebuild_multiple's spec values read["hg18", hg19"]— a missing opening quote, threetimes, which YAML parses as the string
hg19". Fixed.How to test the changes?
Both fixes are red-to-green against the new unit assertions in
test_input_models.py.License