24.2 linter fallout

[bernt-matthias]

Describe the bug

Nearly half of the IUC tool repos wont pass the TestsCaseValidation linter: https://github.com/galaxyproject/tools-iuc/pull/6590 But, many are just due to tests not specifying conditionals. Just had a quick look

• Something seems wrong with the output assertions in tools/spapros/evaluation.xml
• Found that linting tools-iuc/tools/ncbi_entrez_eutils/ takes a lot of memory (but could be something else)

In general I found that it will be quite hard for tool developers to interpret this errors.

Wondering also about the precise meaning of tests won't run on a modern Galaxy tool profile version

If they are all (or many of them) true positives I'm really worried how to fix this. Seems like a huge effort to touch all those tools.

Galaxy Version and/or server at which you observed the bug Galaxy Version: 24.2

[jmchilton]

Wondering also about the precise meaning of tests won't run on a modern Galaxy tool profile version

Starting with profile 24.2, we don't load tests that cannot be exactly matched against a valid schema for running the tool. This means all the parameters must match the API for the tool precisely. This should catch missing required options, any parameter misspelled, parameters not in the right part of the tree, etc..

I think this catches many important of classes of errors in linting that would only be catchable at runtime previously and some stray parameters and such that wouldn't be caught at runtime. This should speed up tool development but I agree the Pydantic output leaves a lot to be desired and I'm not sure how to address the problem.

Seems like a huge effort to touch all those tools.

I mean we should only touch the tools before we update them right? I wouldn't go through and fix things preemptively based on these issues.

tools/spapros/evaluation.xml

I'll take a look at this today.

[jmchilton]

Seems like a huge effort to touch all those tools.

I mean arguably we don't need a tool version update for fixing the test cases? Do we have a best practice around that? I don't mind spending a couple days trying to fix that tools if the reviews will be quick and we don't bump the tool version.

[jmchilton]

tools/spapros/evaluation.xml

This looks like a valid linting issue to me. In test case #6 - the conditional method test parameter is set to plot_marker_corr. The parameter use_marker_corr is only available in the conditional if the method is set to plot_summary - it doesn't appear in there for this plot_marker_corr value. You're setting a stray invalid parameter - I think we should catch this and now we can. If you were designing your test with that feature in mind - it could be very frustrating that it did not do anything I think.

[bernt-matthias]

tools/spapros/evaluation.xml

What I meant is this:

+Linting tool /home/berntm/projects/tools-iuc/tools/spapros/evaluation.xml
.. WARNING: Test 1: failed to validate assertions. Validation errors are [252 validation errors for RootModel[List[Union[Annotated[Union[has_line_model, has_line_matching_model, has_n_lines_model, has_text_model, has_text_matching_model, not_has_text_model, has_n_columns_model, attribute_is_model, attribute_matches_model, element_text_model, element_text_is_model, element_text_matches_model, has_element_with_path_model, has_n_elements_with_path_model, is_valid_xml_model, xml_element_model, has_json_property_with_text_model, has_json_property_with_value_model, has_h5_attribute_model, has_h5_keys_model, has_archive_member_model, has_size_model, has_image_center_of_mass_model, has_image_channels_model, has_image_depth_model, has_image_frames_model, has_image_height_model, has_image_mean_intensity_model, has_image_mean_object_size_model, has_image_n_labels_model, has_image_width_model], FieldInfo(annotation=NoneType, required=True, discriminator='that')], has_line_model_nested, has_line_matching_model_nested, has_n_lines_model_nested, has_text_model_nested, has_text_matching_model_nested, not_has_text_model_nested, has_n_columns_model_nested, attribute_is_model_nested, attribute_matches_model_nested, element_text_model_nested, element_text_is_model_nested, element_text_matches_model_nested, has_element_with_path_model_nested, has_n_elements_with_path_model_nested, is_valid_xml_model_nested, xml_element_model_nested, has_json_property_with_text_model_nested, has_json_property_with_value_model_nested, has_h5_attribute_model_nested, has_h5_keys_model_nested, has_archive_member_model_nested, has_size_model_nested, has_image_center_of_mass_model_nested, has_image_channels_model_nested, has_image_depth_model_nested, has_image_frames_model_nested, has_image_height_model_nested, has_image_mean_intensity_model_nested, has_image_mean_object_size_model_nested, has_image_n_labels_model_nested, has_image_width_model_nested]]]
0.tagged-union[has_line_model,has_line_matching_model,has_n_lines_model,has_text_model,has_text_matching_model,not_has_text_model,has_n_columns_model,attribute_is_model,attribute_matches_model,element_text_model,element_text_is_model,element_text_matches_model,has_element_with_path_model,has_n_elements_with_path_model,is_valid_xml_model,xml_element_model,has_json_property_with_text_model,has_json_property_with_value_model,has_h5_attribute_model,has_h5_keys_model,has_archive_member_model,has_size_model,has_image_center_of_mass_model,has_image_channels_model,has_image_depth_model,has_image_frames_model,has_image_height_model,has_image_mean_intensity_model,has_image_mean_object_size_model,has_image_n_labels_model,has_image_width_model].has_image_width.width
  Assertion failed, Invalid type found 3253 [type=assertion_error, input_value='3253', input_type=str]
0.tagged-union[has_line_model,has_line_matching_model,has_n_lines_model,has_text_model,has_text_matching_model,not_has_text_model,has_n_columns_model,attribute_is_model,attribute_matches_model,element_text_model,element_text_is_model,element_text_matches_model,has_element_with_path_model,has_n_elements_with_path_model,is_valid_xml_model,xml_element_model,has_json_property_with_text_model,has_json_property_with_value_model,has_h5_attribute_model,has_h5_keys_model,has_archive_member_model,has_size_model,has_image_center_of_mass_model,has_image_channels_model,has_image_depth_model,has_image_frames_model,has_image_height_model,has_image_mean_intensity_model,has_image_mean_object_size_model,has_image_n_labels_model,has_image_width_model].has_image_width.delta
  Assertion failed, Invalid type found 2 [type=assertion_error, input_value='2', input_type=str]
0.has_line_model_nested.has_line
  Field required [type=missing, input_value={'that': 'has_image_width...': '3253', 'delta': '2'}, input_type=dict]
0.has_line_model_nested.that
  Extra inputs are not permitted [type=extra_forbidden, input_value='has_image_width', input_type=str]
0.has_line_model_nested.width
  Extra inputs are not permitted [type=extra_forbidden, input_value='3253', input_type=str]
0.has_line_model_nested.delta
  Extra inputs are not permitted [type=extra_forbidden, input_value='2', input_type=str]
0.has_line_matching_model_nested.has_line_matching
...

Then it goes on for pages.

[jmchilton]

tools-iuc/tools/ncbi_entrez_eutils/ takes a lot of memory (but could be something else)

It looks like it is elink and the new parameter validation. It can be run in isolation with galaxy-tool-test-case-validation from galaxy-tool-util.

galaxy-tool-test-case-validation ~/workspace/tools-iuc/tools/ncbi_entrez_eutils/elink.xml

It takes a few minutes on my laptop. I assume pydantic validation against the model generated from db_db_link_macro would take a long time. It is thousands of lines of conditional logic - I guess the structural type matching against that could be tough.

[jmchilton]

Yup - I see the problem with that for sure. We don't use the XML typing when converting those assertions into YAML. The XSD will be correct for these but the assertion models will fail. I'll figure out how to fix that.

[bernt-matthias]

Anyway .. Let me just highlight that I definitely welcome the new linter.

But I'm afraid that the transition will be quite rough for all the tool repos .. besides IUC. Let's say one person can fix 10 tool repos a day .. then that person will need to spend more than a month on IUC. But, we can of course silence the linter repo-wise.

Starting with profile 24.2, we don't load tests that cannot be exactly matched against a valid schema for running the tool.

This means that for tool profile 24.2 this will be an error and (more important) testing goes as before for smaller profiles?

[jmchilton]

This means that for tool profile 24.2 this will be an error and (more important) testing goes as before for smaller profiles?

Testing will work as normal for earlier tool profile versions - yes.

And there should be a clear failure for newest tool profile versions when running Planemo but I haven't run all the pieces together yet. I think you're the first person to use Planemo with the new version of tool-util.

[bernt-matthias]

I think you're the first person to use Planemo with the new version of tool-util.

I always symlink tool-util in my venv to my Galaxy dev directory .. :)