Make access to parameters in sections also require full path

[bernt-matthias]

So far the access to values of parameter in sections did not require to specify the full access path.

For

<section name="sec">
   <param name="par">
</section>

in format_source just par was used. With the change the access is format_source sec|par.

In cheetah $sec.par is already used.

The problem was discovered for an automatic tool conversion where two parameters with the same name existed where one was in a section and the other wasn't. Then one did overwrite the other.

[mvdbeek]

That's a nice test, fixing this probably also fixes #9486

[bernt-matthias]

Cool. Do you know if this worked in the past and just broke or if this never worked?

[mvdbeek]

You're not supposed to duplicate parameter names (if they're not the same thing, as in conditional switches), so there is a chance it never worked, but some years ago we also reworked the param dict lookup for unqualified parameter names that could have affected this.

[bernt-matthias]

I understand and would never do this when crating wrappers manually.

But, I stumbled over this when autogenerating wrappers for OpenMS .. so I don't have any influence on the parameter names .. or need quite a bit of additional code which I would like to avoid.

[mvdbeek]

sure, and that's why I like your test. clearly we don't want to mask input_data_1 with test_section .input_data_1

[bernt-matthias]

Good thing is that the test fails: Dataset metadata verification for [file_ext] failed, expected [fastqsanger] but found [fastqsolexa]

But from my test that brought be to this bug I would have expected Dataset metadata verification for [file_ext] failed, expected [fastqsanger] but found [data]

[bernt-matthias]

Hey @mvdbeek .. this gets a bit tricky and I could need a bit of feedback.

Seemingly visit_input_values did not add prefixes for sections and conditionals so far. That's why the input in the new test was masked. My changes would change this and require that the full name with prefixes is necessary for all attributes referring to inputs (so far I'm aware of format_source and structured_like.

I guess this would break a few tools in its current state (quick check at IUC confirms this).

Also there are some side effects which I do not understand yet (e.g. the py3 api tests).

I'm thinking about a good way to fix this.

• one could add both the qualified and the unqualified name here

  galaxy/lib/galaxy/tools/actions/__init__.py

  Line 64 in 9ba1f45

  def _collect_input_datasets(self, tool, param_values, trans, history, current_user_roles=None, dataset_collection_elements=None, collection_info=None):

• I also remember that in tests with sections / conditionals it is possible address the inputs with and without the section or conditional name (i.e. section|id or just id). But I'm not sure if I'm right. If so then maybe we can add the same flexibility here in the same way?

What's your take on this?

[bernt-matthias]

Reformulated title and descriptions and opened a branch on IUC https://github.com/bernt-matthias/tools-iuc/tree/topic/test-format_source for testing. Tests are running here https://github.com/bernt-matthias/tools-iuc/actions/runs/83804958

[bernt-matthias]

Seems that $section.param already worked, at least that scheme is used all over IUC as far as I have seen.

Maybe it was only format_source (and other attributes referring to other params) that did not use the full path. Question is what else relies on the visit_input_values foo.

[bernt-matthias]

The change seems to have some side effect on workflows (corresponding API tests should be fixed) and conditionals (totally forgot that conditionals are also affected)

There was a comment (likely) from @jmchilton and it seems that the behaviour is now as desired

galaxy/lib/galaxy_test/api/test_workflows.py

Line 2957 in 802ddaf

# Wish it was qualified for conditionals but it doesn't seem to be. -John

[jmchilton]

I thought it was intentional that sections don't require the prefix addition? Won't this break things?

[bernt-matthias]

With respect to tools this seems not to be the case. I just tested the complete IUC repo against this branch (https://github.com/bernt-matthias/tools-iuc/actions/runs/83804958).. All tests passed. I checked some tools and found that in cheetah the access is already $section.parameter.
With this respect the change improves consistency.

Main question seems to be where visit_input_values is called and if this functionality is covered sufficiently by tests. Seems to be only 5 places in the code.

[mvdbeek]

I thought it was intentional that sections don't require the prefix addition? Won't this break things?

It will and we shouldn't change the test tools.

in format_source just par was used. With the change the access is format_source sec|par.

Can we not make both work ?

[bernt-matthias]

I thought it was intentional that sections don't require the prefix addition?

Do you recall the idea behind this intention? On a first glance it seems inconsistent to require prefix in cheetah but not in other places.

It will

Maybe :). So far the good news is that all tests here pass .. as well as all IUC tools -- even if I do not understand why in parts: e.g. https://github.com/galaxyproject/tools-iuc/blob/7f0dbcf650d20acd80d7082bdae9759d51a1480c/tools/sickle/sickle.xml .. does not use prefix addition for conditionals (admittedly as documented: https://docs.galaxyproject.org/en/master/dev/schema.html#tool-outputs-data) .. and should fail for this branch .. hrm.

and we shouldn't change the test tools.

For identifier_in_conditional.xml I thinks the cheetah was just wrong, since it accessed a parameter in a conditional without using the conditional, i.e. it was $input1 instead of $outer_cond.input1. I was surprised that this actually worked.

Can we not make both work ?

Good question. The first thing that came to my mind is to tweak callback helper but for this one would need to define and implement an failure case (exception) for the callback functions. E.g. try the new behavior and if it fails spit out a warning and try the old behavior.

But I guess this idea is much more complicated to implement than it sounds.

[mvdbeek]

Do you recall the idea behind this intention?

It's the old default. Just because this isn't widely used in modern tool doesn't mean we can break this. Of course fully-prefixed is better.

Maybe :)

Case in point are the test tools.

I thinks the cheetah was just wrong, since it accessed a parameter in a conditional without using the conditional, i.e. it was $input1 instead of $outer_cond.input1. I was surprised that this actually worked.

Yes, that used to work and I insist that this must continue working. If you think there is no way to have both working we can make the new behavior conditional on a new profile version.

[bernt-matthias]

I guess we should put the change to identifier_in_conditional.xml to a separate PR (if at all .. do not understand why this worked before).

You can add a profile here: https://github.com/jmchilton/galaxy/blob/b1fc8430bb6b743954b2221d2141f016d0d1e4c9/test/unit/workflows/test_modules.py#L443

Should we add tests here for the new profile? Currently I just added the default profile.

I thought it was intentional that sections don't require the prefix addition? Won't this break things?

It seems that structured_like already was supposed to work like this from 18.09 6cfe588 but I'm not sure if it really did because this was not covered by tests (as far as I can see).

[mvdbeek]

Yeah, let's not change test/functional/tools/identifier_in_conditional.xml. The rest looks good to me, I don't think we need extra profile tests given that you have the high-level framework tests.

[bernt-matthias]

The remaining two workflow api tests should pass now again after I undid the changes.

Considering the source comment of @jmchilton # Wish it was qualified for conditionals but it doesn't seem to be. -John I'm wondering if we should test also the profile version? If yes what do you think is the easiest way to do this over there?

Do I need to create duplicates of the tests using a mapper2 tool xml for the 21.01 profile?

[bernt-matthias]

But in the end it seems a odd/wrong to me that access to variables in workflows might be inconsistent from tool to tool .. depending on their corresponding profile.

[mvdbeek]

I think that (the PJA rename) should be straightforward to fix if we always pass in the fully-prefixed versions. I've experimented a bit with this, and I think we can get away with something like

diff --git a/lib/galaxy/tools/evaluation.py b/lib/galaxy/tools/evaluation.py
index 6ca6482b09..696d593b63 100644
--- a/lib/galaxy/tools/evaluation.py
+++ b/lib/galaxy/tools/evaluation.py
@@ -432,7 +432,18 @@ class ToolEvaluator:
                     # Remove key so that new wrapped object will occupy key slot
                     del param_dict[key]
                     # And replace with new wrapped key
-                    param_dict[wrap_with_safe_string(key, no_wrap_classes=ToolParameterValueWrapper)] = wrap_with_safe_string(value, no_wrap_classes=ToolParameterValueWrapper)
+                    wrapped_value = wrap_with_safe_string(value, no_wrap_classes=ToolParameterValueWrapper)
+                    # add fallback for tools < 21.01
+                    if self.tool.profile < 21.01:
+                        key_fragments = key.split('|')
+                        for i in range(1, len(key_fragments)):
+                            key_fragment = "|".join(key_fragments[(i):])
+                            wrapped_key = wrap_with_safe_string(key_fragment, no_wrap_classes=ToolParameterValueWrapper)
+                            if wrapped_key not in param_dict:
+                                param_dict[wrapped_key] = wrapped_value
+                            else:
+                                param_dict[wrapped_key] = wrapped_value
+                    param_dict[wrap_with_safe_string(key, no_wrap_classes=ToolParameterValueWrapper)] = wrapped_value

which means we don't have to pass the profile version around, instead we always generate the fully prefixed version internally and generate the fallback for the param_dict.
However, something doesn't quite work there yet, I'll spend a bit more time on this.

[mvdbeek]

I spent the better part of today looking at this, and I suspect this isn't quite working as it should. It breaks for profile >= 21.01 and multiple="true" iteration in the command block, and it only works in other cases because the fallback that should only be used in rare cases (https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/tools/evaluation.py#L288) saves the day.

The code itself looks good and results in fully prefixed job parameters and JobToInputDatasetAssociations, which I think is a big plus if we can use that for all tool executions. I think we can handle unprefixed parameters in the params_dict. Given this fixes a bug, I will branch without this, but we should come back to it.

#9493 (comment)

[mvdbeek]

fwiw that is where I stopped today: d1445c6

[martenson]

I am not sure how it happened but it seems this feature has gotten to the xsd before it was merged.

edit: xsd modified here

[bernt-matthias]

I am not sure how it happened but it seems this feature has gotten to the xsd before it was merged.

edit: xsd modified here

True. I can't recall either. We should make sure that we either finish this PR or revert the change for 21.09

[bernt-matthias]

Hey @martenson .. just checked: The change is not in dev, but I can find it in the 21.05 branch. I'm confused how this happened. Should I revert it only on the 21.05 branch?

[bernt-matthias]

fwiw that is where I stopped today: d1445c6

Just thought to rebase this to keep the PR up to date. Should I just include your commit before doing this?

[martenson]

Well that is confusing, one can hope that this was the only thing that is stuck there in the limbo. :/

[nsoranzo]

Hey @martenson .. just checked: The change is not in dev, but I can find it in the 21.05 branch. I'm confused how this happened. Should I revert it only on the 21.05 branch?

I think it's also in dev?

https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/tool_util/xsd/galaxy.xsd#L2524-L2530