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 workflow parameter validators. #19092

Merged
merged 14 commits into from
Nov 15, 2024

Conversation

jmchilton
Copy link
Member

Synchronized the working implementation in #18781 with the parameter models introduced in #19027. Extended it to include min/max on integer and float parameters and to synchronize language and help with regular expression inputs form the rule builder.

Screenshot 2024-10-31 at 11 58 51 AM Screenshot 2024-10-31 at 11 58 37 AM Screenshot 2024-10-31 at 1 09 15 PM

How to test the changes?

(Select all options that apply)

License

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

@jmchilton jmchilton force-pushed the min_max_regex branch 2 times, most recently from dc63ce8 to 2507b03 Compare November 8, 2024 12:21
@jmchilton
Copy link
Member Author

The API test failure is legit - no clue why though. GALAXY_TEST_TOOL_CONF="test/functional/tools/sample_tool_conf.xml" GALAXY_CONFIG_OVERRIDE_CONDA_AUTO_INIT=false pytest -s lib/galaxy_test/api/test_workflows.py::TestWorkflowsApi::test_export_invocation_ro_crate_adv. I'll dig into it Monday.

@@ -7034,12 +7034,14 @@ def test_subworkflow_import_order_maintained(self, history_id):
outer_input_1:
type: int
default: 1
optional: true
Copy link
Member Author

Choose a reason for hiding this comment

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

@mvdbeek Is this being required now a regression?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I guess we might have to introduce something to track the meaning of default but no explicit optional ? Like the profile version we have for tools ?

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to revert 545d783 and I open a separate PR to work on this ?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we never exposed it in the UI and it was only available by hacking ga files for most - I'm fine with just forcing a new behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll take you up on that but let me know if you want help with that piece. I've pulled out various related things - my test fixes and those two commits and pushed the result back into this branch. The pre-rebase version of my version of this branch is backed up at https://github.com/jmchilton/galaxy/tree/backup_min_max_regex_before_strip_default_handling.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's a good point, this is only ambiguous in the gxformat2 language where a default value would make the parameter optional previously. So you're okay with having to specify optional: true in gxformat2 to create optional inputs ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah - I think so 🤔.

jmchilton and others added 13 commits November 13, 2024 09:50
Which allows building repeats and conditionals.
We don't need to replciate this logic just for workflow parameters.
- Allow specifying a min/max for integer and float parameters.
- Swap language from regex to "Regular Expression" and synchronize help with regular expression fields in rule builder.
- Extend regular expression help.
- Improve default message for in_range validator. Probably a reversion in some ways introduced with galaxyproject#19027.
@jmchilton
Copy link
Member Author

I don't love the UI but the model we're saving into the DB is pretty flexible, well tested, and can accommodate whatever so I think this is fine. We can I think iterate on the UI without this one locking us in (the way we have to build up the form on the backend and send that representation over the wire instead of a designed/modeled API is the only reason I have any doubt on that).

@jmchilton jmchilton marked this pull request as ready for review November 14, 2024 14:44
@github-actions github-actions bot added this to the 25.0 milestone Nov 14, 2024
@mvdbeek mvdbeek merged commit 215b459 into galaxyproject:dev Nov 15, 2024
57 of 58 checks passed
@mvdbeek
Copy link
Member

mvdbeek commented Nov 15, 2024

Thanks a lot @jmchilton!

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