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

Tool form section tag #35

Merged
merged 16 commits into from Apr 10, 2015
Merged

Conversation

guerler
Copy link
Contributor

@guerler guerler commented Mar 23, 2015

This adds a section grouping tag to tool form parameters as discussed in https://trello.com/c/KxlQK0FB/587-ui-tools-provide-methods-for-visually-grouping-parameters.

The format is: section name="NAME" title="TITLE" expanded="True/False"

@dannon
Copy link
Member

dannon commented Mar 23, 2015

This is an automated message. Thanks for your contribution, a Trello card to track this issue has been created. Apply this patch for testing.

 - Add example tool for testing (section.xml).
 - Implement popular flat group style test cases for section elements (first example in section.xml).
 - Implement nested groupping style test cases for section elements (section example in section.xml).
@jmchilton
Copy link
Member

Opened PR to your branch implementing tool testing functionality.

@guerler
Copy link
Contributor Author

guerler commented Mar 24, 2015

Awesome. Thx.

@jmchilton
Copy link
Member

A couple things - this looks like this will end the old tool form? I am nervous about that because we haven't implemented an upload widget for the new tool form - right?

Also have you checked out templates/webapps/tool_shed/repository/tool_form.mako - you may want to issue a warning or something there that the tool uses components not displayed. Ultimately it would be awesome to use the new tool form there as well - how hard to you think it would be to render the form without runtime history data and a Galaxy API?

@guerler
Copy link
Contributor Author

guerler commented Mar 25, 2015

Thx for the comments. I think that the upload tool parameter should be deprecated since it is not a clean solution. Instead users should upload files through the regular UI. Regarding the tool shed form mako, the best solution is probably to extend the current mako and then later this year consider to rewrite the tool shed UI without makos at all.

@martenson
Copy link
Member

We can also remove the form-preview functionality from the TS and just redirect (or show a modal) to a specialized usegalaxy.org/preview?tool_id=xxxxxxxx service.

@martenson
Copy link
Member

👍

…ol_form_section_tag

Conflicts:
	lib/galaxy/tools/__init__.py
@dannon
Copy link
Member

dannon commented Apr 9, 2015

@guerler -- This disables the old tool form completely, removing the 'toolform_upgrade' conditional from tool_form.mako. Is that intentional, or is it left over from your testing?

@@ -3,455 +3,22 @@
${h.js("libs/bibtex", "libs/jquery/jquery-ui")}
${h.css('base', 'jquery-ui/smoothness/jquery-ui')}

## skip new tool form code if disabled
%if util.string_as_bool(trans.app.config.get('toolform_upgrade', True)):
Copy link
Member

Choose a reason for hiding this comment

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

(the line in question, per my comment below -- intentional?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this disables the old tool form for good. Currently we are building both tool forms in parallel. Since sections are not supported in the old tool form, we can either integrate them or disable the old tool form for good which would allow us to clean up the code and remove the associated makos all together. However, as John pointed out the latter has the consequence that the upload and ftp tool parameters will be deprecated.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, got it -- I hadn't put it together that John was talking about this when he said 'end the old tool form', somehow. Super short deprecation warning, we probably need to handle this and not just purge it all quite yet?

@dannon
Copy link
Member

dannon commented Apr 9, 2015

Discussed with @guerler offline a bit, but this can't go as-is since it removes capabilities that still need to be reproduced in the new tool form, and it disables the old one. Path forward is going to be to implement the missing features in the new tool form so there's no functionality loss and everything is fully backwards compatible, and then (and only then) disable the old tool form and remove it from the codebase.

@jmchilton
Copy link
Member

So this PR made me nervous - but this statement "no functionality loss and everything is fully backwards compatible" also makes me nervous in the other direction. Is composite uploads what need to be preserved or is having a upload widget that can be embedded a tool?

@dannon
Copy link
Member

dannon commented Apr 9, 2015

I think what we were leaning towards was an upload widget. @guerler -- want to comment w/ plans?

@jmchilton
Copy link
Member

I really have very mixed feelings - but if the intention is to continue to support composite uploads - we should restore a link to the old upload form on main - because currently there is no link and we are telling people these datatypes are no longer supported https://biostar.usegalaxy.org/p/11371/. No reason to lose the goodwill now and then have to support these indefinitely anyway :).

@bgruening
Copy link
Member

@jmchilton this biostar link was hopefully only referring to PLED files and the tools developed at this time. We need to upload composite datatypes also in GalaxyP - wiff - scream, run, hide!

@jmchilton
Copy link
Member

Believe me @bgruening I want a usable wiff datatype in Galaxy - I am just not sure two or three datatypes warrant another 1 or 2 months of @guerler's time. They may be - I really have no clue though.

@jxtx
Copy link
Contributor

jxtx commented Apr 9, 2015

I think composite datatypes are important.

@jmchilton
Copy link
Member

@bgruening
Copy link
Member

@jmchilton this I can not judge. Just wanted to point out that there are some rare cases.

@jmchilton
Copy link
Member

@jxtx Did you intend to say "uploading of composite datatypes are important" - that is the stakes of this - not removing them completely.

@jxtx
Copy link
Contributor

jxtx commented Apr 9, 2015

I think it is probably worth spending the effort to make them uploadable now.

@jmchilton
Copy link
Member

@jxtx Awesome - if you think it is time well spent - my conscience is clear.

@guerler
Copy link
Contributor Author

guerler commented Apr 9, 2015

So it seems that we have two issues here: 1. the composite uploads and 2. the upload/ftp tool parameter. For the composite uploads I wonder if we can provide a tool which combines files uploaded through the regular upload modal into composite datatypes? Regarding the upload/ftp tool parameter (which is only used in one case within the toolshed) I would look out for a workaround i.e. by simply replacing them with regular dataset selectors and asking the user to use the regular upload modal maybe?

@guerler
Copy link
Contributor Author

guerler commented Apr 10, 2015

Regarding 2.: Since no one in the toolshed (one outcommented attempt excluded) is using neither the file nor the ftpfile tool parameters, we actually might want to deprecate them. The alternative would be to fix and rewrite them such that they properly work incl. in the workflow editor. Instead we could ask the community if anyone is using these parameter types outside the tool shed. Depending on the number of responses, users should be able to simply adjust their tools by using regular dataset selectors.

@guerler
Copy link
Contributor Author

guerler commented Apr 10, 2015

Since this seems to be a separate discussion. I have re-activated the tool form fall back option for this PR.

jmchilton added a commit that referenced this pull request Apr 10, 2015
@jmchilton jmchilton merged commit 38b06a7 into galaxyproject:dev Apr 10, 2015
@jmchilton
Copy link
Member

Awesome - thanks @guerler.

@natefoo
Copy link
Member

natefoo commented Apr 10, 2015

@guerler - For the composite uploads I wonder if we can provide a tool which combines files uploaded through the regular upload modal into composite datatypes

This seems like a pretty reasonable way to do it that'll fall in line with the way collections are built.

@blankenberg
Copy link
Member

Composite datasets and collections are different in so many ways, allowing upload of the individual parts and then combining them is probably not feasible, since it is quite possible for the individual parts to not be uploadable e.g., random generic "binary content not allowed", whereas if uploaded as the composite datatype the individual files together are valid for the datatype.

Also, the case of uploading a composite datatype piecemeal will waste disk space/usage, and part of the reason that we have composite datasets in many cases is that the individual items "are not useful by themselves".

Also, any tool to combine them will need to use the same/similar underlying logic as the current uploading of these -- that is allowing setting metadata, modifying the names of files, etc.

@blankenberg
Copy link
Member

In reference to no longer supporting certain input types, the ToolShed is not the end-all-be all of tool availability. Many tools written by individuals may not ever make it to the ToolShed, but that doesn't mean they should stop working, or that we should force them to change their tool to use different parameter types.

We have also stated in the past that we would maintain backwards compatibility, specifically with respect to tool definitions.

@jmchilton
Copy link
Member

Point well articulated on composite datatypes - I was trying to explain that last week out-of-band but you did a much better job.

As for preserving type="file" indefinitely with the exact same API - that is worrying to me. I know from experience that modifying upload.xml to do non-trivial things requires modifications to upload.py. Do you also consider everything in upload.py part of Galaxy's stable API? Is any modified upload.py suppose to work forever against any future version of Galaxy? Here are the Galaxy imports it makes -

from galaxy import eggs
# need to import model before sniff to resolve a circular import dependency
import galaxy.model
from galaxy.datatypes.checkers import *
from galaxy.datatypes import sniff
from galaxy.datatypes.binary import *
from galaxy.datatypes.images import Pdf
from galaxy.datatypes.registry import Registry
from galaxy import util
from galaxy.datatypes.util.image_util import *
from galaxy.util.json import *

From the outside, my impression was that type="file" was never a serious part of the tool API - it was bolted on to allow a very specific kind of upload process to work as tools. If we want to get serious about allowing customization to the upload process that is great - lets do that within the new framework - not try to preserve the old API that we don't even know is being used anywhere.

@natefoo
Copy link
Member

natefoo commented Apr 13, 2015

From the outside, my impression was that type="file" was never a serious part of the tool API - it was bolted on to allow a very specific kind of upload process to work as tools.

This is right.

@blankenberg
Copy link
Member

Unfortunately, they are listed on https://wiki.galaxyproject.org/Admin/Tools/ToolConfigSyntax

type="color"

(New as of 15.03 release.) ...

type="genomebuild"

...

type="hidden"

...

type="hidden_data"

...

type="baseurl"

...

type="file"

...

type="ftpfile"

...

type="library_data"

... 

@jmchilton
Copy link
Member

I think the entirety of its documentation being literally ... is a very good indication that it wasn't a serious part of the API.

@blankenberg
Copy link
Member

This documentation is poor and we've gotten quite better at documenting things, but I'd be remiss to not point out that some documentation does exist.

That being said, I don't actually like these parameters to be used by 'non-built in' tools (e.g. upload). Tools should be working on Datasets and not uploaded files.

@natefoo
Copy link
Member

natefoo commented Apr 13, 2015

I suspect they were only placed on that page because someone pulled every possible value out with the intent that they should be addressed somehow at some point. However, I don't believe we ever intended for any of those file params to be used directly in a tool.

@guerler
Copy link
Contributor Author

guerler commented Apr 13, 2015

This is a pretty good discussion. Thx for all the input.

@guerler guerler deleted the tool_form_section_tag branch May 18, 2015 13:44
dannon referenced this pull request in dannon/galaxy May 11, 2017
Fix require inclusion scoping in analysis.js
sveinugu added a commit to elixir-oslo/proto that referenced this pull request Apr 6, 2018
joachimwolff pushed a commit to joachimwolff/galaxy that referenced this pull request Jan 16, 2020
jmchilton pushed a commit that referenced this pull request Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants