galaxy
galaxy copied to clipboard
galaxyproject
Make access to parameters in sections also require full path
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.
That's a nice test, fixing this probably also fixes https://github.com/galaxyproject/galaxy/issues/9486
Cool. Do you know if this worked in the past and just broke or if this never worked?
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.
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.
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
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]
So here is a possible fix .. at least this test is working now as expected. Maybe this will have side effects, since inputs in sections are now referenced as section|input.
Not sure if this is actually a bug in the visitor:
- somehow the
parent_prefixis empty when it probably shouldn't so I returnname_prefixhttps://github.com/galaxyproject/galaxy/blob/972fd28601dbb8815d51e6fd4118c948ee2ae314/lib/galaxy/tools/parameters/init.py#L128 - alternatively the calling functions could use
prefixed_name
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 https://github.com/galaxyproject/galaxy/blob/9ba1f4550cfa9d25074dd0764fa7189aed10eb36/lib/galaxy/tools/actions/init.py#L64
-
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|idor justid). 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?
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
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.
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
https://github.com/galaxyproject/galaxy/blob/802ddafd561175f4ad957206ee057454b5257715/lib/galaxy_test/api/test_workflows.py#L2957
I thought it was intentional that sections don't require the prefix addition? Won't this break things?
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.
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
parwas used. With the change the access is format_sourcesec|par.
Can we not make both work ?
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.
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
$input1instead 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.
I agree completely. The profile option seems to be the best solution. You already increased the milestone to 20.09 which seems realistic.
Will think about if its possible to change the behavior without breaking stuff.
Just thought about implementing a profile version. Seems that the function is called in quite a few places:
- lib/galaxy/managers/workflows.py
- lib/galaxy/workflow/modules.py
- lib/galaxy/tools/init.py
- lib/galaxy/tools/evaluation.py
- lib/galaxy/tools/parameters/meta.py
Is there a better way then introducing a new profile parameter (in the function itself and probably quite a lot of the calling functions, and in the functions calling them, etc).
Does someone has an idea for my last question?
Is there a better way then introducing a new profile parameter (in the function itself and probably quite a lot of the calling functions, and in the functions calling them, etc).
Btw. xrefs between input parameters seem to work in yet another way and xref to parameters in sections seem not to work at all https://github.com/galaxyproject/galaxy/pull/11026
Hey @mvdbeek . I'm making good progress with the profile version. The only problem seems to be two calls in https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/workflow/modules.py where tool is a Bunch and I do not know what to do there yet.
I also extended the unit tests for the function to cover both profile versions. I'm still not entirely sure if this is "kind of a bug"
See the lines 66 ff: https://github.com/galaxyproject/galaxy/blob/c4947ff3dcc95711bc60537e64928bff0ff41d17/lib/galaxy/tools/parameters/init.py#L66
Parameters e, g, and h are in the section d, but this is inconsistently represented in the prefix (i.e. d is not in the prefix of e, but it is for g and h). More precisely the old implementation skipped the parent level which makes prefix+name != prefixed_name.
I guess this would become apparent for sections or condiationals with 2 or more nesting levels and output parameter referring to them .. quite unlikely ..
Left are to undo the changes to the test tools and replace them by new tests. I guess we need a profile version for each of them, or?
The only problem seems to be two calls in https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/workflow/modules.py where
toolis aBunchand I do not know what to do there yet.
Where exactly are you seeing this ?
The only problem seems to be two calls in https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/workflow/modules.py where
toolis aBunchand I do not know what to do there yet.Where exactly are you seeing this ?
In the failing unit tests:
https://app.circleci.com/pipelines/github/galaxyproject/galaxy/12501/workflows/215d048b-9fd6-42f2-856f-d70a58b47d27/jobs/123787
You can add a profile here: https://github.com/jmchilton/galaxy/blob/b1fc8430bb6b743954b2221d2141f016d0d1e4c9/test/unit/workflows/test_modules.py#L443
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 https://github.com/galaxyproject/galaxy/commit/6cfe5880e13e0f704782afb72ebafd7fac3d45d4 but I'm not sure if it really did because this was not covered by tests (as far as I can see).
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.
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?
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.
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.