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

[23.0] Fix implicit converters with optional parameters #16133

Merged
merged 2 commits into from May 24, 2023

Conversation

mvdbeek
Copy link
Member

@mvdbeek mvdbeek commented May 22, 2023

In this case

<param argument="-chrom" type="text" value="" optional="true" label="Restrict output to given chromosome">

Fixes:

KeyError: 'chrom'
  File "galaxy/tools/__init__.py", line 1909, in handle_single_execution
    rval = self.execute(
  File "galaxy/tools/__init__.py", line 2005, in execute
    return self.tool_action.execute(
  File "galaxy/tools/actions/__init__.py", line 392, in execute
    ) = self._collect_inputs(tool, trans, incoming, history, current_user_roles, collection_info)
  File "galaxy/tools/actions/__init__.py", line 327, in _collect_inputs
    inp_data, all_permissions = self._collect_input_datasets(
  File "galaxy/tools/actions/__init__.py", line 268, in _collect_input_datasets
    tool.visit_inputs(param_values, visitor)
  File "galaxy/tools/__init__.py", line 1748, in visit_inputs
    visit_input_values(self.inputs, values, callback)
  File "galaxy/tools/parameters/__init__.py", line 224, in visit_input_values
    callback_helper(
  File "galaxy/tools/parameters/__init__.py", line 159, in callback_helper
    new_value = callback(**args)
  File "galaxy/tools/actions/__init__.py", line 191, in visitor
    input_datasets[prefix + input.name] = process_dataset(value)
  File "galaxy/tools/actions/__init__.py", line 126, in process_dataset
    data = data.get_converted_dataset(trans, target_ext, target_context=parent, history=history)
  File "galaxy/model/__init__.py", line 4211, in get_converted_dataset
    self.datatype.convert_dataset(
  File "galaxy/datatypes/data.py", line 795, in convert_dataset
    job, converted_datasets, *_ = converter.execute(trans, incoming=params, set_output_hid=visible, history=history)
  File "galaxy/tools/__init__.py", line 2005, in execute
    return self.tool_action.execute(
  File "galaxy/tools/actions/__init__.py", line 453, in execute
    params=wrapped_params.params,
  File "galaxy/tools/parameters/wrapped.py", line 43, in params
    self.wrap_values(self.tool.inputs, params, skip_missing_values=not self.tool.check_values)
  File "galaxy/tools/parameters/wrapped.py", line 58, in wrap_values
    value = input_values[input.name]

in https://sentry.galaxyproject.org/share/issue/4f2171798f214f7ba8b947eaaebe2776/

(Please replace this header with a description of your pull request. Please include BOTH what you did and why you made the changes. The "why" may simply be citing a relevant Galaxy issue.)
(If fixing a bug, please add any relevant error or traceback)
(For UI components, it is recommended to include screenshots or screencasts)

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.

@github-actions github-actions bot added this to the 23.0 milestone May 22, 2023
Copy link
Contributor

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

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

Looks good to me. Was just wondering if the order of the if-elif branches matters. And maybe you can explain how the test works.

@mvdbeek
Copy link
Member Author

mvdbeek commented May 23, 2023

The test is round-tripping from bigwig to wig to bigwig, exercising the code that didn't work.
input_name is the primary input, that's not one of the dependencies, and the rest is potentially optional parameters, so i think it needs to be that way.

@bernt-matthias
Copy link
Contributor

Are tools contained in the galaxy code base tested (#15229)?

@mvdbeek
Copy link
Member Author

mvdbeek commented May 23, 2023

No, I don't think it's worth the CI time to test all of them. I can add this one to the framework tests.

@bernt-matthias
Copy link
Contributor

test/functional/test_toolbox_pytest.py::TestFrameworkTools::test_tool[wig_to_bigWig/1.1.1-0] SKIPPED (tool in skiplist) [ 57%]
test/functional/test_toolbox_pytest.py::TestFrameworkTools::test_tool[wig_to_bigWig/1.1.1-1] SKIPPED (tool in skiplist) [ 57%]
test/functional/test_toolbox_pytest.py::TestFrameworkTools::test_tool[wig_to_bigWig/1.1.1-2] SKIPPED (tool in skiplist) [ 58%]
test/functional/test_toolbox_pytest.py::TestFrameworkTools::test_tool[wig_to_bigWig/1.1.1-3] SKIPPED (tool in skiplist) [ 58%]

@bernt-matthias
Copy link
Contributor

maybe remove

2>&1 || echo "Error running wigToBigWig." >&2

@mvdbeek
Copy link
Member Author

mvdbeek commented May 24, 2023

Don't really want to touch this, I'm just gonna do a separate framework test tool.

…arams

Fixes:
```
KeyError: 'chrom'
  File "galaxy/tools/__init__.py", line 1909, in handle_single_execution
    rval = self.execute(
  File "galaxy/tools/__init__.py", line 2005, in execute
    return self.tool_action.execute(
  File "galaxy/tools/actions/__init__.py", line 392, in execute
    ) = self._collect_inputs(tool, trans, incoming, history, current_user_roles, collection_info)
  File "galaxy/tools/actions/__init__.py", line 327, in _collect_inputs
    inp_data, all_permissions = self._collect_input_datasets(
  File "galaxy/tools/actions/__init__.py", line 268, in _collect_input_datasets
    tool.visit_inputs(param_values, visitor)
  File "galaxy/tools/__init__.py", line 1748, in visit_inputs
    visit_input_values(self.inputs, values, callback)
  File "galaxy/tools/parameters/__init__.py", line 224, in visit_input_values
    callback_helper(
  File "galaxy/tools/parameters/__init__.py", line 159, in callback_helper
    new_value = callback(**args)
  File "galaxy/tools/actions/__init__.py", line 191, in visitor
    input_datasets[prefix + input.name] = process_dataset(value)
  File "galaxy/tools/actions/__init__.py", line 126, in process_dataset
    data = data.get_converted_dataset(trans, target_ext, target_context=parent, history=history)
  File "galaxy/model/__init__.py", line 4211, in get_converted_dataset
    self.datatype.convert_dataset(
  File "galaxy/datatypes/data.py", line 795, in convert_dataset
    job, converted_datasets, *_ = converter.execute(trans, incoming=params, set_output_hid=visible, history=history)
  File "galaxy/tools/__init__.py", line 2005, in execute
    return self.tool_action.execute(
  File "galaxy/tools/actions/__init__.py", line 453, in execute
    params=wrapped_params.params,
  File "galaxy/tools/parameters/wrapped.py", line 43, in params
    self.wrap_values(self.tool.inputs, params, skip_missing_values=not self.tool.check_values)
  File "galaxy/tools/parameters/wrapped.py", line 58, in wrap_values
    value = input_values[input.name]
```

in https://sentry.galaxyproject.org/share/issue/4f2171798f214f7ba8b947eaaebe2776/
Copy link
Contributor

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

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

Excellent :)

@mvdbeek mvdbeek merged commit 0901ad7 into galaxyproject:release_23.0 May 24, 2023
33 of 35 checks passed
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.

None yet

2 participants