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

Implement default locations for data and collection parameters. #14955

Merged
merged 5 commits into from
Nov 6, 2023

Conversation

jmchilton
Copy link
Member

@jmchilton jmchilton commented Nov 8, 2022

Works for both files and collections. Workflow defaults override tool defaults.

This is not strictly better or worse than #14938 / #13110, but to my eyes this is both cleaner than that approach and brings in more CWL (the raw_to_galaxy here is a cleanup and generalization of a CWL-branch-ism). Both approaches commit Galaxy to doing some long term, difficult things - I think by not adding a new tool form type, not adding new models, not adding new ways users can have data associated with their account, etc... this approach is a bit more conservative. Both approaches do some things with workflow state that make me uncomfortable and this approach adds some tool features - but I feel good about the tool features (they are useful, tested, and feel both cohesive and 'galaxy').

TODO:

  • Fix up the UI
  • Unit test case to ensure this only works for non-default, non-multi data parameters.
  • Implement XSD once syntax is finalized.

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.

Works for both files and collections. Workflow defaults override tool defaults.

TODO:

- Unit test case to ensure this only works for non-default, non-multi data parameters.
- Implement XSD once syntax is finalized.
@jmchilton
Copy link
Member Author

Initial rebase for the 2023 biohackathon done... next step is I think synchronizing this with the conditionals work. They both brought in similar code from the CWL branch in different ways.

@jmchilton
Copy link
Member Author

XSD needs some progress made also - luckily test failures caught this.

not ok 27 test/functional/tools/collection_nested_default.xml
    -:10: element collection: Schemas validity error : Element 'collection': This element is not expected. Expected is one of ( change_format, filter, discover_datasets, actions ).
    - fails to validate
test/functional/tools/collection_nested_test.xml
ok 28
test/functional/tools/collection_optional_param.xml
ok 29
test/functional/tools/collection_paired_conditional_structured_like.xml
ok 30
test/functional/tools/collection_paired_default.xml
not ok 31 test/functional/tools/collection_paired_default.xml
    -:8: element element: Schemas validity error : Element 'element', attribute 'location': The attribute 'location' is not allowed.
    -:9: element element: Schemas validity error : Element 'element', attribute 'location': The attribute 'location' is not allowed.
    - fails to validate
test/functional/tools/collection_paired_structured_like.xml

@jmchilton
Copy link
Member Author

Test tools now validate and I fixed the typing issue I discovered in the conditional branch of the code. I'd be interesting to see how this works the CWL branch now.

@jmchilton jmchilton marked this pull request as ready for review November 2, 2023 17:04
@github-actions github-actions bot added this to the 23.2 milestone Nov 2, 2023
Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @jmchilton!

@@ -2094,6 +2099,11 @@ def __init__(self, tool, input_source, trans=None):
self._parse_options(input_source)
# Load conversions required for the dataset input
self.conversions = []
self.default_object = input_source.parse_default()
if self.optional and self.default_object is not None:
raise ParameterValueError(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like a perfectly legitimate case, just like default values on optional parameters. It's a UI thing that we can't reset them to None/null/undefined (didn't check, it might actually be possible now) ... but let's get this in, this is a slightly off-topic discussion.

Copy link
Member Author

@jmchilton jmchilton Nov 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't disagree in principle but perhaps better to add explicitly after the fact with a lot of intentionality and with explicit tests - UI + API + Cheetah Tests.

@jmchilton jmchilton merged commit ea0fe80 into galaxyproject:dev Nov 6, 2023
51 of 52 checks passed
@nsoranzo nsoranzo deleted the default_uris_2 branch November 6, 2023 15:17
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.

3 participants