-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix collection mapping to allow subcollection mapping over multi-data inputs. #6255
Fix collection mapping to allow subcollection mapping over multi-data inputs. #6255
Conversation
lib/galaxy/tools/parameters/basic.py
Outdated
if match: | ||
subcollection_type = None | ||
if multiple: | ||
subcollection_type = self._history_query(trans).can_map_over(hdca).collection_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something isn't quite right here, I see this traceback when building the tool form:
galaxy.web.framework.decorators ERROR 2018-06-01 10:45:31,618 Uncaught exception in exposed API method:
Traceback (most recent call last):
File "/Users/mvandenb/src/galaxy/lib/galaxy/web/framework/decorators.py", line 281, in decorator
rval = func(self, trans, *args, **kwargs)
File "/Users/mvandenb/src/galaxy/lib/galaxy/webapps/galaxy/api/tools.py", line 113, in build
return tool.to_json(trans, kwd.get('inputs', kwd))
File "/Users/mvandenb/src/galaxy/lib/galaxy/tools/__init__.py", line 1883, in to_json
self.populate_model(request_context, self.inputs, state_inputs, tool_model['inputs'])
File "/Users/mvandenb/src/galaxy/lib/galaxy/tools/__init__.py", line 1954, in populate_model
tool_dict = input.to_dict(request_context)
File "/Users/mvandenb/src/galaxy/lib/galaxy/tools/parameters/basic.py", line 1871, in to_dict
subcollection_type = self._history_query(trans).can_map_over(hdca).collection_type
AttributeError: 'bool' object has no attribute 'collection_type'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - the other place this is used in that file it verifies it is truthy before using it - my bad. I'll throw in a continue in that loop if it is false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you've rebased the commit, but something must've gone wrong again.
I suppose this should be as in 880c021 ?
It is jarring to users that works for collection inputs but not multiple data inputs, especially because the IUC discourages collection inputs when multiple data inputs would work.
86272cd
to
b908ea4
Compare
I'm not sure this does the right thing (all the time). If I use the So in the screenshot I've used collection 13 twice as input to the But maybe I'm thinking about this the wrong way and multiple="true" should always completely reduce entire collections. What I thought would happen I could always achieve with a collection input and a regular output, right ? |
Clearly my rebase was wrong, sorry about. I'll bring in 880c021 - that looks right. I tried two inputs at once with the same tool and it works for me I think - any chance you didn't rebuild the client? What you are describing is what happens without the client being rebuilt. If you rebuilt the client I'll do some more testing. Also - since you are doing deep testing I should mention the form doesn't do any validation. So like it makes sense that picking multiple lists reduces all the lists together at once and that should work or that picking a nested list does this subcollection mapping thing you expect - but if you mix and match the two or specify multiple nested lists ... things are going to break and the sharp change of behavior has no representation in UI between lists and nested lists (this PR mirrors the problem #2467 with subcollection mapping over collection paramaeters). |
Cool! I saw the client being rebuilt, but my chrome is sometimes a bit stubborn with getting the new client if not opening the console and disabling the cache. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super awesome, thanks a lot @jmchilton !
Calling it a fix with the hope of back porting to release_18.05 at some point - it is jarring to users that works for collection inputs but not multiple data inputs, especially because the IUC discourages collection inputs when multiple data inputs would work.
Fixes #6236 and #5875.