Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix docs for format_source #19330

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

bernt-matthias
Copy link
Contributor

Document that reference to parameters in format_source in sections and conditionals needs to be qualified with |. Collection elements can be referred to using element access syntax.

To verify this I added a log statement here: lib/galaxy/tools/actions/init.py printing format_source, input_datasets and input_dataset_collections. Here we look at a test for a tool with a pair paired_input in a conditional single_paired (IUC's fastp tool). Access would be via 'single_paired|paired_input:

galaxy.tools.actions ERROR 2024-12-16 16:14:29,475 [pN:main.1,p:2622874,tN:WSGI_0] format_source=None input_datasets={'single_paired|paired_input1': <galaxy.model.HistoryDatasetAssociation(65) at 0x7f7c610de860>, 'single_paired|paired_input2': <galaxy.model.HistoryDatasetAssociation(66) at 0x7f7c610dd000>} input_dataset_collections={'single_paired|paired_input': [(<galaxy.model.HistoryDatasetCollectionAssociation(16) at 0x7f7c60e45fc0>, False)]}

For the single ended input this looks analogous. Bottom line the dictionaries do not contain the unqualified inputs. In the fastp tool additional output actions were implemented https://github.com/galaxyproject/tools-iuc/blob/a80e3e4aa3a40970af507bf9119cf7f1c2ffb336/tools/fastp/macros.xml#L73 .. likely because the unqualified format_source did not work.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

Reference to parameters in sections and conditionals needs to be qualified
with `|`. Collection elements can be referred to using element access
syntax.
@github-actions github-actions bot changed the title [24.2] fix docs for format_source fix docs for format_source Dec 16, 2024
@github-actions github-actions bot added this to the 25.0 milestone Dec 16, 2024
@mvdbeek
Copy link
Member

mvdbeek commented Dec 17, 2024

Seems kind of weird to have documented wrong examples ? Are you sure those specific examples are broken and it's not a general issue with tool profile versions or actual bugs ?

@bernt-matthias
Copy link
Contributor Author

Seems kind of weird to have documented wrong examples ? Are you sure those specific examples are broken and it's not a general issue with tool profile versions or actual bugs ?

I'm not sure. I could add a test checking if it works currently.

the test for the nesting case accidentally works since the
[legacy_mapping](https://github.com/galaxyproject/galaxy/blob/16ec912385b59429b1e87cebb817ec012b02ec4e/lib/galaxy/tools/parameters/wrapped.py#L43) contains a wrong mapping.
with the output2 we can now test this.

with output3 we now also test that the legacy behavior works.
@bernt-matthias
Copy link
Contributor Author

Seems kind of weird to have documented wrong examples?

Independent of the question if the syntax is correct, I would suggest to document the desired syntax :) To me it seems that this is fully qualified paths to the parameters, or?

But the behavior is really weird, as the updated test shows:

  • Unqualified access (input1 as done in output3)
    • works for parameters in a conditional (because legacy_mapping={'input1': 'cond|input1'} in test1)
    • but not for deeper nested parameters
  • Qualified access using just one nesting level (cond|input1 as done in output1)
    • works for a parameter in a conditional (because here it is fully qualified)
    • works also for deeper nested cases because of legacy_mapping={'cond|input1': 'cond|inner_cond|input1'}
  • Fully qualified always works as expected

My expectation would have been that unqualified input always works legacy_mapping={'input1': ...}. But it seems that this was deliberately implemented this way: xref #15978

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants